On Sun, Dec 13, 2015 at 4:31 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> On Sun, Dec 13, 2015 at 11:10 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html