Hi Peff, On Fri, 6 Apr 2018, Jeff King wrote: > On Tue, Apr 03, 2018 at 06:28:42PM +0200, Johannes Schindelin wrote: > > > While a neat theoretical construct, state machines are hard to read. In > > this instance, it does not even make a whole lot of sense because we are > > more interested in flags, anyway: has the section been seen? Has the key > > been seen? Does the current section match the key we are looking for? > > > > Besides, the state `SECTION_SEEN` was named in a misleading way: it did > > not indicate that we saw the section matching the key we are looking > > for, but it instead indicated that we are *currently* in that section. > > > > Let's just replace the state machine logic by clear and obvious flags. > > > > This will also make it easier to review the upcoming patches to use the > > newly-introduced `event_fn` callback of the config parser. > > I think this is probably a good direction. But one thing state machines > can help with is keeping the state to a manageable size. With 3 bits of > flags, we now have 8 possible states, up from the previous 4. > > Clearly some of those are nonsensical (can you be in key_seen without > section_seen? I'd think not), but it's up to the code to interpret and > reset those manually. That is true. On the other hand, it is easy to miss incorrect state transitions in state machines (or to miss unused states). > I'll defer to your judgement, though, on this making things for the > future patches more readable. You spend a lot more time poking at it > than I have. The original reason to get rid of the state machine was: I did not need the states any more in the end. Since the section name is set via the event stream we now know in the config_fn whether we are in the correct section or not. I also liked the fact that it was much easier to reason about correct code: "Did I catch all the states that apply here?" is a hairier question than "Is this flag true?" Thanks, Dscho
