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
