On Mon, Dec 14, 2015 at 3:25 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> 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.
Ya could do that, I have the series ready then, I was gonna work on the
'enum atom_type' but I'll work on top of this, like you suggested.
Thanks.
--
Regards,
Karthik Nayak
--
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