Re: [PATCH/RFC 00/10] ref-filter: use parsing functions

> On Sun, Dec 13, 2015 at 4:31 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Sun, Dec 13, 2015 at 11:10 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> If the intention is to rid that inner loop of much of the expensive
>>> processing, then wouldn't we want to introduce an enum of valid atoms
>>> which is to be a member of 'struct used_atom', and have
>>> populate_value() switch on the enum value rather than doing all the
>>> expensive strcmp()s and starts_with()?
>>>
>>>     enum atom_type {
>>>         AT_REFNAME,
>>>         AT_OBJECTTYPE,
>>>         ...
>>>         AT_COLOR,
>>>         AT_ALIGN
>>>     };
>>>
>>>     static struct used_atom {
>>>         enum atom_type atom;
>>>         cmp_type cmp;
>>>         union {
>>>             char *color; /* parsed color */
>>>             struct align align;
>>>             enum { ... } remote_ref;
>>>             struct {
>>>                 enum { ... } portion;
>>>                 unsigned int nlines;
>>>             } contents;
>>>             int short_objname;
>>>         } u;
>>>     } *used_atom;
>>>
>>> In fact, the 'cmp_type cmp' field can be dropped altogether since it
>>> can just as easily be looked up when needed by keeping 'enum
>>> atom_type' and valid_atoms[] in-sync and indexing into valid_atoms[]
>>> by atom_type:
>>>
>>>     struct used_atom *a = ...;
>>>     cmp_type cmp = valid_atoms[a->atom].cmp_type;
>>>
>>> As a bonus, an 'enum atom_type' would resolve the problem of how
>>> starts_with() is abused in populate_value() for certain cases
>>> (assuming I'm understanding the logic), such as how matching of
>>> "color" could incorrectly match some yet-to-be-added atom named
>>> "colorize".
>>
>> This actually makes sense to me, especially how we can eliminate most of
>> the starts_with() in populate_value(). Well then I guess I'll work on this and
>> see what I can come up with, Thanks for the review :)
>
> The 'enum atom_type' approach doesn't necessarily need to be part of
> this patch series; it may make sense to layer it atop the current
> series, which is already long enough to make review onerous. Patches
> in the current series are things you would want to do anyhow for the
> 'enum atom_type' approach, and there isn't really anything in the
> current series which should present a roadblock for layering 'enum
> atom_type' atop. Plus, by layering 'enum atom_type' atop, you get a
> chance to polish the current series (based upon review comments) and
> let it settle down, which should make working on 'enum atom_type'
> easier.