Re: Minor Comments on Gmake Internals

On 12/17/21 20:22, Jon Forrest wrote:

> 1) variable.c/free_variable_set() looks like it also frees the 
> variable_set_list in the variable_set passed as an argument.
> Should it be modified to only free the variable_set? Another
> function could then be created called free_variable_set_list()
> which could itself call free_variable_set().

My take on this is a bit different: free_variable_set is poorly named 
and could be renamed to free_variable_set_list. There seems to be no 
need for a separate free_variable_set function.

> 2) The comment at line 367 in variable.c says there are only two special 
> variables (e.g. .VARIABLES and .TARGETS). However, I see in main.c 
> around line 1319 where .VARIABLES is defined and made special, but the 
> line below it that does the same thing for .TARGETS is commented out.
> Instead the variable .RECIPEPREFIX is defined and made special.
> Shouldn't the comment in variable.c be fixed?

Yes, it appears that .TARGETS is a circa 2002 idea that never got 
published. The source code and/or commentary mentions .TARGETS twelve 
times and perhaps these should all be looked at.

> 3) There's a misplaced comment about popping the top set off the current
> variable_set_list in variable.c on lines 666 and 667. It should be 
> before line 712.

Good catch.

Paul already covered (4).

> 5) I've been programming in C since ~1977 but there is some code
> in gmake of a style I've never seen before. For example

Those examples look OK to me, except ....

> o = variable_buffer_output (o, p, p1 != 0 ? (size_t) (p1 - p) : strlen 
> (p) + 1);

I'd write just "p1 - p" rather than "(size_t) (p1 - p)" since I prefer 
to avoid unsigned types, for better runtime checking with gcc 
-fsanitize=undefined.

> # define SIZE_MAX ((size_t)~(size_t)0)

'((size_t) -1)' is technically more correct here, for the rare and 
now-obsolete platforms that use ones' complement or signed magnitude and 
that don't already define SIZE_MAX as required by C99. I doubt whether 
GNU Make supports these obsolete platforms anyway, for other reasons.