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

> 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:
>>> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +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]).
>>
>> 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?
>
> If I understand your question correctly, it sounds like you're asking
> about a memory micro-optimization. From an architectural standpoint,
> it's cleaner for the entity which allocates a resource to also release
> it. In this case, show_ref_array_item() allocates the strbuf, thus it
> should be the one to release it.
>
> And, although we shouldn't be worrying about micro-optimizations at
> this point, if it were to be an issue, resetting the strbuf via
> strbuf_reset(), which doesn't involve slow memory
> deallocation/reallocation, is likely to be a winner over repeated
> strbuf_release().
>

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

>>>>         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.
>
> Hmm, where were you seeing that warning? The first use of 'atomv'
> following its declaration is in the get_ref_atom_value() below, and
> (as far as the compiler knows) that should be setting its value.
>

>>>>                 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()?
>
> I don't know. I don't have a proper high-level overview of the
> functionality yet to say if that is a good name or not (which is one
> reason I didn't suggest an alternative).
>

>>> 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.
>
> Meh. print_value() might be better named format_value(). emit() might
> become append_literal() or append_non_atom() or something.
>

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