Ideally the condition for allocating more memory would have been
(old_len + 2 > bp->cap) since that's the actually intended wording. But
GCC 5 neglected to generate good code for that on x86_64, so be it.
The tricky detail here is that the current node from the iteration is
removed if it is no longer needed.
The Lst_FindDatum that has been removed was both inefficient and
misleading since it could never return null, yet there was a null check
for it. The callback API from Lst_ForEachUntil would have required to
define a custom struct for passing this parameter to the callback
function, in addition to the parent node.
Inlining the whole Lst_ForEach and passing the list node as a parameter
is much more obvious.
This protects against very simple memory allocation bugs such as
migrating Lst_ForEachUntil to Lst_ForEach without remembering that
Lst_ForEachUntil can handle the situation where the current list node is
removed from the list, but Lst_ForEach cannot. This happens in
Make_ExpandUse, for example.
Before August 2020, the Lst library passed null pointers through. This
was a confusing design pattern that has been removed since. Now the Lst
functions fail fast on null pointers.
The 'targets' list is one of the few places where there is indeed an
optional list that may sometimes be null. Back then, there was not
enough inline documentation to understand when the targets list was null
and when it wasn't.
Now that the documentation is there, the redundant and thereby
misleading null checks are no longer useful.
Printing a node does not modify the structure of the node, therefore the
additional housekeeping of Lst_ForEachUntil is not needed here.
Inlining the callback function also removes a lot of pointer stuff that
is more difficult to read than necessary.
This avoids the extra local function and a few conversions to void
pointers, to gain additional type safety.
The code in Compat_RunCommand does not modify gn->commands structurally,
therefore it does not need the extra complexity of Lst_ForEachUntil. It
does have access to a list node to exactly this list. This list node is
only used to set the command to NULL after processing it, not for
removing the node from the list.
There is a crucial difference between these functions, in that
Lst_ForEachUntil can cope with a few concurrent modifications while
iterating over the list. This is something that Lst_ForEach doesn't do.
This difference led to a crash very early in NetBSD's build.sh.
These functions made the code larger than necessary. The prev and next
fields are published intentionally since navigating in a doubly-linked
list is simple to do and there is no need to wrap this in a layer of
function calls, not even syntactically. (On the execution level, the
function calls had been inlined anyway.)
The 3 callers of this function passed different flags, and these flags
led to code paths that almost did not overlap.
It's a bit strange that GCC 5 didn't get that, and even marking the
function as inline did not produce much smaller code, even though the
conditions inside that function were obviously constant. Clang 9 did a
better job here.
But even for human readers, inlining the function and then throwing away
the dead code leads to much easier code.
This pattern of squeezing completely different code into a single
function has already occurred in a different part of make, though I
don't remember where exactly.
The previous API had complicated rules for the cases in which the single
function returned NULL or what it did. The flags for that function were
confusing since passing TARG_NOHASH would create a new node even though
TARG_CREATE was not included in that bit mask.
Splitting the function into 3 separate functions avoids this confusion.
It also reveals several places where the complicated API led to
unreachable code. Such code has been removed.
This variable has served at least 27 years bringing unnecessary
redundancy to the code. It was already redundant at 1993-03-21, when
the code was imported to NetBSD.
When there is a dependency group at the end of a top-level makefile,
this dependency group is not finished properly. This allows to add
further commands to the targets of this dependency group, which was not
intended.
Since at least 1993-03-21, adding other makefiles in a .MAKEFILES
dependency has invoked undefined behavior because the command line
arguments were copied directly into the global makefiles variable,
without a proper strdup. Shortly after that, the word list created by
Str_Words (formerly brk_string) was freed.
This applies to both the -f and the -v and -V options. Luckily it is an
edge case to use these options in .MAKEFLAGS at all.
The -T option had already been fixed at 2000-12-30, but not the other
options.
Parsing a single shell command from a line does not belong in
Parse_File, its proper place is in Parse_Line. Having the whole
detailed code inline in Parse_File is even more confusing.
