Re: [PATCH v2 08/15] config: introduce an optional event stream while parsing

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Fri, 6 Apr 2018, Jeff King wrote:

> On Tue, Apr 03, 2018 at 06:28:29PM +0200, Johannes Schindelin wrote:
> 
> > This extends our config parser so that it can optionally produce an
> > event stream via callback function, where it reports e.g. when a
> > comment was parsed, or a section header, etc.
> > 
> > This parser will be used subsequently to handle the scenarios better
> > where removing config entries would make sections empty, or where a
> > new entry could be added to an already-existing, empty section.
> 
> Nice, it looks like this didn't end up being too bad to go in this
> direction. It seems like this is an optional "also emit the events here"
> function you can set.

Yes.

> I think in the long run we could actually just always emit the events to
> this function. And then we could wrap that to provide an interface that
> matches the existing callbacks (just an event-stream callback that sees
> EVENT_ENTRY and calls the sub-callback).

Well, not precisely. The event stream was implemented in a minimal
fashion, in particular *not* emitting enough information in the event
stream for that. To keep things as little intrusive as possible, the
CONFIG_EVENT_ENTRY event is only emitted *after* the config_fn is called,
and at that point we do not even know the key and the value any more.

I fear that it would make the code quite a bit more complicated to change
it in the way you suggested.

Side note: a slightly ugly aspect of my patch series is that the
CONFIG_EVENT_SECTION event *also* does not provide the interesting
information (in this case, the section name), but that it has to be
inferred from the cf->var field (which is file-local to config.c, and
which has been set to the section name followed by a single '.' at that
point). Again, this keeps the diff simpler to review, and that's why I did
it that way.

> But that might end up quite a pain, since we have a zillion entry points
> into the config parser, making wrapping tough. So I'm perfectly happy to
> stop here for now.

Right.

> > +static inline int do_event(enum config_event_t type,
> > +			   struct parse_event_data *data)
> 
> I'm not sure if "inline" here is a good idea, as it seems to get called
> quite a few times. If we're trying to make things fast, bloating the
> instruction cache may have the opposite effect.

Good point.

The reason I declared this as inline function was that I test whether
either data->opts or data->opts->event_fn are NULL, and whether we are
continuing to look at whitespace, for early returns from that function.
Which I wanted to avoid doing in a hot function (I'd rather skip calling
the function if it is pointless to call it).

However, the config code is hardly performance-critical, as we do not
expect to parse hundreds of kilobytes, right? So that "inline" was a
premature optimization.

Thanks,
Dscho