From: Theo Buehler <tb@theobuehler.org>
Subject: Re: patch: tweak make "magic variables" handling
To: Marc Espie <marc.espie.openbsd@gmail.com>
Cc: tech@openbsd.org
Date: Wed, 26 Nov 2025 11:09:38 +0100

On Tue, Nov 25, 2025 at 12:03:33PM +0100, Marc Espie wrote:
> The idea is that it's shorter to special-case ${@D} and the likes
> (two characters variables ending in D or F) instead of having a weird
> idx encoding.
> 
> Cons: 
> - this yields an extra "ext" parameter to classify_var
> Pros:
> - the weird index encoding vanishes
> - no need for special treatment if we add more similar variables.
> - drastically reduces the size of the switch (and the modulo shrinks from
> 82 to 36)
> - code will recognize constructs like ${?D} and ${?F}, which puts us in
> line with FreeBSD and NetBSD bmake, and also with gnu make.

This generally reads fine to me. A question and a nit below.

> Index: Makefile
> ===================================================================
> RCS file: /build/data/openbsd/cvs/src/usr.bin/make/Makefile,v
> diff -u -p -r1.65 Makefile
> --- Makefile	4 Sep 2023 11:35:11 -0000	1.65
> +++ Makefile	22 Nov 2025 09:42:30 -0000
> @@ -27,7 +27,7 @@ CLEANFILES+=generate generate.o regress.
>  CLEANFILES+= varhashconsts.h condhashconsts.h nodehashconsts.h
>  
>  # may need tweaking if you add variable synonyms or change the hash function
> -MAGICVARSLOTS=82
> +MAGICVARSLOTS=36

Is there a reason you chose 36 and not 34? According to the comment in
classify_var() MAGICSLOTS1 should be the smallest constant yielding
distinct case values and 'obj/generate 1 34' doesn't find a collision.

>  MAGICCONDSLOTS=65
>  
>  varhashconsts.h: generate

[...]

> Index: var.c
> ===================================================================

[...]

> @@ -279,14 +244,25 @@ static bool parse_base_variable_name(con
>   * GLOBAL_INDEX if name is not dynamic. Set up *pk for further use.
>   */
>  static int
> -classify_var(const char *name, const char **enamePtr, uint32_t *pk)
> +classify_var(const char *name, const char **enamePtr, uint32_t *pk, char *ext)
>  {
>  	size_t len;
> +	uint32_t k;
> +	*ext = 0;
> +

Nit: Whitespace is a bit weird. Should this use '*ext = '\0';'?

>  
>  	*pk = ohash_interval(name, enamePtr);
>  	len = *enamePtr - name;