Lst_Find is called with a "comparison" function that returns the integer
0 if the desired node is found. This leads to confusion since there are
so many different return value conventions for int, such as 0/1 for
mimicking false/true, -1/0 as in close(2), and the sign as in strcmp(3).
This API is much easier to understand if the "comparison" function is
not called a comparison function (since that is too close to strcmp),
but a "match" function that just returns a boolean.
In Lst_FindFromB, the node argument may be null. This deviates from the
other Lst functions, which require Lst and LstNode to generally be
non-null. In this case it is useful though to make the calling code
simpler.
In arch.c, this makes a lot of the previous documentation redundant.
In cond.c, the documentation is reduced a little bit since it had
already been cleaned up before. It also removes the strange negation
from CondFindStrMatch.
In dir.c, the documentation collapses as well.
In main.c, separating the ReadMakefile function from the callbacks for
Lst_FindB allows the former to get back its natural function signature,
with proper types and no unused parameters.
To catch any accidental mistakes during the migration from Lst_Find to
Lst_FindB, the code can be compiled with -DUSE_DOUBLE_BOOLEAN, which
will complain about incompatible function pointer types.
This is quite hard to trigger in a real-life scenario since it requires
precise timing.
Instead, I created /tmp/sys.mk and ran "./make -m /tmp -f /dev/null" in
the debugger, after setting a breakpoint in this line:
ln = Lst_Find(sysMkPath, ReadMakefile, NULL);
Once this line was reached, I removed /tmp/sys.mk to make ReadMakefile
fail. Just adding a few parse errors won't help since ReadMakefile only
fails if the file cannot be found.
Having Boolean aliased to int creates ambiguities since int is widely
used. Allow to occasionally compile make with -DUSE_DOUBLE_BOOLEAN to
check that the type definitions still agree.
Var_Subst never returns NULL.
In Main_ExportMAKEFLAGS, don't compare ints with booleans.
In MainParseArgs, use char for the current character. First, that's
more precise and correct, and second, it makes debugging easier for
those who don't know the ASCII table by heart.
The old name implied that the function would read multiple files, which
was not the case.
The comment above that function was highly confusing. It's not that the
function returns a boolean, but rather 0 or non-zero, and 0 means that
Lst_Find should stop searching.
One of the next refactorings will be to make Lst_Find return the first
list node for which the function returns TRUE. This will reduce the
confusion about the several functions called SomethingP in suff.c. The
P suffix means to return TRUE or FALSE, not 0 or non-zero.
The migration from null-passing Lst functions to argument-checking Lst
functions is completed.
There were 2 surprises: The targets list may be NULL, and in Dir_AddDir,
the path may be NULL. The latter case is especially surprising since
that function turns into an almost-nop in that case. This is another
case where probably 2 independent functions have been squeezed into a
single function. This may be improved in a follow-up commit.
All other lists were fine. They were always defined and thus didn't
need much work.
In most cases the Lst functions are only called when the arguments are
indeed valid. It's not guaranteed though, therefore each function call
needs to be analyzed and converted individually.
While here, remove a few statements that were only useful when the Lst
functions handled circular lists.
The outdated type name FreeProc had been renamed to LstFreeProc.
Casting the function free to it is not necessary since the type of this
function is already exactly the correct type. Anything else would be
undefined behavior anyway.
The uninitialized sufflist in Suff_ClearSuffixes was ok until now
because the Lst functions had silently skipped any calls with invalid
arguments. This silent skipping is a good argument to have strict
argument validation since it detects these unintended control flows.
This change ensures that there is actually something added to the list.
Lst_AtEnd had silently skipped the addition if the list was invalid
(null pointer), which was not intended in these cases. The "(void)" is
assumed to mean "I know that this cannot fail", while it could also mean
"I don't care whether something actually happened".
Running "./build.sh -j6 tools" still succeeds after this change,
therefore chances are very low that this change breaks anything. If
there is any change, it's an obvious assertion failure. There is no
silent change in behavior though.
The list library had probably been imported from a general-purpose
library that also supported circular lists. These are not used by make
though.
After replacing Lst_Init(FALSE) with Lst_Init(), only a single call to
Lst_Init remained with a non-constant argument, and that was in
Lst_Concat, which was to be expected.
The new functions have a simpler interface, and str_concat3 is even more
general-purpose, since the middle string is no longer required to be
exactly of length 1.
Iterating the command output backwards was dangerous since at the end,
the pointer cp pointed outside of the array. Even without dereferencing
this pointer, this already invokes undefined behavior (C11, 6.5.6p8).
Don't risk anything. Iterating forwards is probably faster anyway, since
it is more common.
This Z had been useful during the migration from int to size_t. This
migration is finished, at least for the Buffer type, so the Z is no
longer necessary.
NetBSD make is intended to be maximally portable, therefore it uses only
C89. This was not declared in the Makefile before.
There are still a few places in parse.c and metachar.c that use
end-of-line comments. These will be fixed in a follow-up commit.
GCC 9 incorrectly claims that the string might not be null-terminated.
Since objdir is a global variable, it is initialized to zero, and the +1
in the size guarantees that this byte is always 0.
Still, using strncpy to initialize a string is a waste of memory access,
since it is enough if only the actual data is copied, without zeroing
out all the remaining bytes.
This change helps to make the various integer types compatible and is a
preparational step for setting WARNS=6 in the Makefile.
The documentation of buf.c has been cleaned up and condensed since it
was mostly redundant, and some statements were even slightly wrong.
All code changes are covered by the existing unit tests, except for the
few lines in for.c around for_var_len. These changes have been reviewed
thoroughly and manually, like all the others in this commit.
Those buffer functions that deal with sizes have been renamed by
appending a Z, to make sure that no function call was accidentally
forgotten. They will be renamed back in a follow-up commit.
As usual, the scope of a few affected variables has been reduced, and
some variables had to be split since they had been incorrectly merged
before.
The order of the arguments to Buf_AddBytes has changed from (mem_len,
mem) to (mem, mem_len), in order to make it consistent with the
functions from the C standard library, such as snprintf.
When parsing variable assignments other than := and if
value contains '$' attempt Var_Subst the same as for :=,
if the value does not parse correctly, we get a fatal error
including file an line number.
This can greatly help with finding the cause of problems.
Reviewed by: christos
The first parameter from Var_Subst had been a literal NULL in all cases.
These have been fixed using this command:
sed -i 's|Var_Subst(NULL, |Var_Subst(|' *.c
The one remaining case was not found because the "NULL," was followed by
a line break instead of a space.
The removed code probably wouldn't have worked as expected anyway.
Expanding a single variable to a literal string would have led to
unexpected behavior for cases like ${VAR:M${pattern}}, in case pattern
would contain an unescaped ':' itself.
Reporting keys on every lookup is overkill unless
playing with a new HASH, so wrap in #ifdef DEBUG_HASH_LOOKUP
Also add some stats at the end so we can see
final size and max chain length - maxchain is a better
variable name than maxlen.
In var.c there are lots of different flag types. To make any accidental
mixture obvious, each flag group gets its own prefix.
The only flag group that is visible outside of var.c is concerned with
evaluating variables, therefore the "e", which replaces the former "f"
that probably just meant "flag".
Allow tracking of max chain length, to see how well the hash
tables are working.
Pull the actual hash operation into a marco so it can be
easily changed - for experimenting.
The current hash, is pretty good.
Reviewed by: christos
