Requesting review of "Eliminate some pointless gymnastics in the config parser."

[Eric S. Raymond]

About eight hours ago I removed some code that looked so stupid that I
now wonder if it was serving some purpose I don't understand.

As the change comment says,

    For some odd reason the code for parsing server clauses created a secondary
    FIFO of attribute/value nodes to be composed into flag bits later, rather
    than simply composing the flag bits immediately.  It worked, but why?

To expand on the strangeness:

create_peer_node() is a parser function that gathers all the information
required to make a peer node from a declaration like, say

server ntp.ubuntu.com iburst xleave ttl 5 maxpoll 128

It expects to get an address node (ntp.ubuntu.com) and a list of flags
or attribute-value pairs; you can sort of diagram it like this

server ntp.ubuntu.com {iburst} {xleave} {ttl 5} {maxpoll 128}

Before my commit, it looked at each item and would do one of two things with it.

(1) Sometimes the item would directly set a field in the new node.  This
is the case with ttl and minpoll.

(2) For flag items, such as iburst and xleave, the iterm would get put
on a newly created list attached to the node.  Later, when some other
function wanted the value of the flag mask for this entry, it would
call a helper that would walk that FIFO and | the values together.

What's puzzling me is: why not just | all those flags together at the
time of the initial parse, and stash that in a flag field?  Why create the
linked list and delay evaluation?

It looks just *stupid*, but now I wonder if I was missing something.
I'nm soliciting anyone to look at ntp_config.c and try to spot a side
effect or assumption that I missed.

(Yes, I tested, and made sure that the new logic sets the "prefer" flag
when it should. Taking that as representative.)
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>