Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Fri, Dec 11, 2015 at 5:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>>> ref-filter: introduce a parsing function for each atom in valid_atom
>>> ref-filter: introduce struct used_atom
>>> ref-fitler: bump match_atom() name to the top
>>> ref-filter: skip deref specifier in match_atom_name()
>>> ref-filter: introduce color_atom_parser()
>>> strbuf: introduce strbuf_split_str_without_term()
>>> ref-filter: introduce align_atom_parser()
>>> ref-filter: introduce remote_ref_atom_parser()
>>> ref-filter: introduce contents_atom_parser()
>>> ref-filter: introduce objectname_atom_parser()
>>
>> It seems that this series had seen quite a good inputs, mostly from
>> Eric. I finished reading it over and I didn't find anything more to
>> add. The patches are mostly good and would be ready once these
>> points raised during the review are addressed, I think
>
> I'm still a bit fuzzy about what this series is trying to achieve. It
> feels like it wants to avoid doing repeated processing of unchanging
> bits of %(foo:bar) atoms for each ref processed, but it only partly
> achieves that goal.
That's very true.
It seems you two already have hashed it out in the downthread, and I
think that is in line with an earlier suggestion by Matthieu to
fully pre-parse in the earlier thread, which was made in response to
(and is much better than) my "let's start with a half-way solution"
in $gmane/279254.
Thanks.
> strcmp()s and starts_with()s in that inner loop, and even the
> unchanging %(color:) argument gets re-evaulated repeatedly, which is
> probably quite expensive.
>
> 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".
--
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