On Sat, Jul 25, 2015 at 4:30 AM, Junio C Hamano <[email protected]> wrote:
> Junio C Hamano <[email protected]> writes:
>
>> Karthik Nayak <[email protected]> writes:
>>
>>> - if (!ref->value) {
>>> - populate_value(ref);
>>> + /*
>>> + * If the atom is a pseudo_atom then we re-populate the value
>>> + * into the ref_formatting_state stucture.
>>> + */
>>> + if (!ref->value || ref->value[atom].pseudo_atom) {
>>> + populate_value(state, ref);
>>> fill_missing_values(ref->value);
>>
> I am not sure why you need to do this. populate_value() and
> fill_missing_values() are fairly heavy-weight operations that are
> expected to grab everything necessary from scratch, and that is why
> we ensure that we do not call them more than once for each "ref"
> with by guarding the calls with "if (!ref->value)".
>
> This change is breaking that basic arrangement, and worse yet, it
> forces us re-read everything about that ref, leaking old ref->value.
>
> Why could this be a good idea?
>
This was required as populate_value() would fill the 'state' but the 'state'
being a static variable would be lost before printing and hence we would
not have the correct values of the 'state' when printing.
> I think populate_value() should not take state; that is the root
> cause of this mistake.
Yes! agreed. atomv should be a better candidate to hold the formatting values.
>
> The flow should be:
>
> - verify_format() looks at the format string and enumerates all
> atoms that will ever be used in the output by calling
> parse_atom() and letting it to fill used_atom[];
>
> - when ref->value is not yet populated, populate_value() is
> called, just once. This uses the enumeration in used_atom[]
> and stores computed value to refs->value[atom];
>
> - show_ref() repeatedly calls find_next() to find the next
> reference to %(...), emits everything before it, and then
> uses the atom value (i.e. ref->value[atom]).
>
> I would expect that the atom value for pseudos like color and align
> to be parsed and stored in ref->value in populate_value() when it is
> called for the first time for each ref _just once_.
>
> "color:blue" may choose to store "blue" as v->s, and "align:4" may
> choose to do "v->ul = 4".
>
> And the code that uses these values should look more like:
>
> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
> struct atom_value *atomv;
>
> ep = strchr(sp, ')');
> if (cp < sp)
> emit(cp, sp);
> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
> &atomv);
> if (atomv->is_pseudo)
> apply_pseudo_state(&state, atomv);
> else
> print_value(&state, atomv);
> }
>
> where apply_pseudo_state() would switch on what kind of pseudo the
> atom is and update the state accordingly, i.e. the "state" munging
> code you added to populate_value() in this patch.
This makes more sense, and avoids the repetitive call to populate_value().
Will implement this.
Thanks
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
