On Fri, Aug 7, 2015 at 11:00 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>>>> It feels strange to assign a local variable reference to state.output,
>>>>> and there's no obvious reason why you should need to do so. I would
>>>>> have instead expected ref_format_state to be declared as:
>>>>>
>>>>> struct ref_formatting_state {
>>>>> int quote_style;
>>>>> struct strbuf output;
>>>>> };
>>>>>
>>>>> and initialized as so:
>>>>>
>>>>> memset(&state, 0, sizeof(state));
>>>>> state.quote_style = quote_style;
>>>>> strbuf_init(&state.output, 0);
>>>>>
>>>>> (In fact, the memset() isn't even necessary here since you're
>>>>> initializing all fields explicitly, though perhaps you want the
>>>>> memset() because a future patch adds more fields which are not
>>>>> initialized explicitly?)
>>>>
>>>> Yea the memset is needed for bit fields evnetually added in the future.
>>>
>>> Perhaps move the memset() to the first patch which actually requires
>>> it, where it won't be (effectively) dead code, as it becomes here once
>>> you make the above change.
>>
>> But why would I need it there, we need to only memset() the ref_formatting_state
>> which is introduced here. Also here it helps in setting the strbuf
>> within ref_formatting_state to {0, 0, 0}.
>
> If you declare ref_formatting_state as shown above, and then
> initialize it like so:
>
> state.quote_style = quote_style;
> strbuf_init(&state.output, 0);
>
> then (as of this patch) the structure is fully initialized because
> you've initialized each member individually. Adding a memset() above
> those two lines would be do-nothing -- it would be wasted code -- and
> would likely confuse someone reading the code, specifically because
> the code is do-nothing and has no value (in this patch). Making each
> patch understandable is one of your goals when organizing the patch
> series; if a patch confuses a reader (say, by doing unnecessary work),
> then it isn't satisfying that goal.
>
> As for the strbuf member, it's initialized explicitly via
> strbuf_init(), so there's no value in having memset() first initialize
> it to {0, 0, 0}. Again, that's wasted code.
Yes, what you're saying is true, if we replace memset() with
state.quote_style = quote_style;
strbuf_init(&state.output, 0);
That does replace the need for memset and make the patch
more self-explanatory.
>
> In a later patch, when you add another ref_formatting_state member or
> two, then you will need to initialize those members too. That
> initialization may be in the form of explicit assignment to each
> member, or it may be the memset() sledgehammer approach, but the
> initialization for those members should be added in the patch which
> introduces them.
>
> It's true that the end result is the same. By the end of the series,
> you'll have memset() above the 'state.quote' and 'state.output'
> initialization lines to ensure that your various boolean fields and
> whatnot are initialized to zero, but each patch should be
> self-contained and make sense on its own, doing just the work that it
> needs to do, and not doing unrelated work. For this patch, the
> memset() is unrelated work. For the later patch in which you add more
> fields to ref_formatting_state(), the memset() is work necessary to
> satisfy that patch's objective, thus belongs there.
I understand what you're saying, it makes sense to have individual patches
only bring in changes related to the patch. This makes a lot of sense.
I will change it up. Thanks :)
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html