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

[Karthik Nayak <karthik.188@gmail.com>]

On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce a ref_formatting_state which will eventually hold the values
>> of modifier atoms. Implement this within ref-filter.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
>> +{
>> +       /* More formatting options to be evetually added */
>> +       strbuf_addbuf(final, state->output);
>> +       strbuf_release(state->output);
>
> I guess the idea here is that you intend state->output to be re-used
> and it is convenient to "clear" it here rather than making that the
> responsibility of each caller. For re-use, it is more typical to use
> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/273094
>

it seems like a smarter way to around this without much overhead But it
was more of to release it as its no longer required unless another modifier atom
is encountered. Is it worth keeping hoping for another modifier atom eventually,
and release it at the end like you suggested below?

>> +}
>> +
>>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>>  {
>>         const char *cp, *sp, *ep;
>> -       struct strbuf output = STRBUF_INIT;
>> +       struct strbuf value = STRBUF_INIT;
>> +       struct strbuf final_buf = STRBUF_INIT;
>> +       struct ref_formatting_state state;
>>         int i;
>>
>> +       memset(&state, 0, sizeof(state));
>> +       state.quote_style = quote_style;
>> +       state.output = &value;
>
> 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);
>

This looks neater, thanks. It'll go along with the previous patch.

> (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.

> This still allows re-use via strbuf_reset() mentioned above.
>
> And, of course, you'd want to strbuf_release() it at the end of this
> function where you're already releasing final_buf.
>

Addressed this above.

>>         for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>> -               struct atom_value *atomv;
>> +               struct atom_value *atomv = NULL;
>
> What is this change about?
>

To remove the warning about atomv being unassigned before usage.

>>                 ep = strchr(sp, ')');
>> -               if (cp < sp)
>> -                       emit(cp, sp, &output);
>> +               if (cp < sp) {
>> +                       emit(cp, sp, &state);
>> +                       apply_formatting_state(&state, &final_buf);
>> +               }
>>                 get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>> -               print_value(atomv, quote_style, &output);
>> +               process_formatting_state(atomv, &state);
>> +               print_value(atomv, &state);
>> +               apply_formatting_state(&state, &final_buf);
>>         }
>>         if (*cp) {
>>                 sp = cp + strlen(cp);
>> -               emit(cp, sp, &output);
>> +               emit(cp, sp, &state);
>> +               apply_formatting_state(&state, &final_buf);
>
> I'm getting the feeling that these functions
> (process_formatting_state, print_value, emit, apply_formatting_state)
> are becoming misnamed (again) with the latest structural changes (but
> perhaps I haven't read far enough into the series yet?).
>
> process_formatting_state() is rather generic.
>

perhaps set_formatting_state()?

> print_value() and emit() both imply outputting something, but neither
> does so anymore.
>

I think I'll append a "to_state" to each of them.

> apply_formatting_state() seems to be more about finalizing the
> already-formatted output.

perform_state_formatting()? perhaps.

-- 
Regards,
Karthik Nayak