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

On Wed, Nov 26, 2025 at 11:09:38AM +0100, Theo Buehler wrote:
> 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.

Yes... I want to be able to support $^ eventually (gnu make extension)

> >  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';'?
0 or '\0' is the same anyway.
> >  
> >  	*pk = ohash_interval(name, enamePtr);
> >  	len = *enamePtr - name;
>