Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine <sunshine@sunshineco.com> 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.

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.