simplify dynamic variable handling a great deal:

first remove all usage of Varq_Append by building the string directly.
then replace `common' handling with specialized handling for dynamic
strings (since they no longer need a buffer). Finally, identify the place
where the variable value needs to be copied because it's going to be free'd
or erased soon, and finally, use simple char* pointers.

Shaves about 80 bytes off every gnode structure, and kills quite a few
unnecessary malloc()s as well.

7 files changed, 131 insertions, 174 deletions

diff --git a/usr.bin/make/arch.c b/usr.bin/make/arch.c
index 70661376d8f..23f6363e6fe 100644
--- a/usr.bin/make/arch.c
+++ b/usr.bin/make/arch.c
@@ -1,5 +1,5 @@
 /*	$OpenPackages$ */
-/*	$OpenBSD: arch.c,v 1.74 2007/11/10 12:51:40 espie Exp $ */
+/*	$OpenBSD: arch.c,v 1.75 2007/11/17 16:39:45 espie Exp $ */
 /*	$NetBSD: arch.c,v 1.17 1996/11/06 17:58:59 christos Exp $	*/
 
 /*
@@ -872,7 +872,7 @@ ArchTouch(const char *archive, const char *member)
 void
 Arch_Touch(GNode *gn)
 {
-	ArchTouch(Varq_Value(ARCHIVE_INDEX, gn), Varq_Value(MEMBER_INDEX, gn));
+	ArchTouch(Var(ARCHIVE_INDEX, gn), Var(MEMBER_INDEX, gn));
 }
 
 /*ARGSUSED*/
@@ -891,8 +891,8 @@ Arch_TouchLib(GNode *gn UNUSED)
 TIMESTAMP
 Arch_MTime(GNode *gn)
 {
-	gn->mtime = ArchMTimeMember(Varq_Value(ARCHIVE_INDEX, gn),
-	     Varq_Value(MEMBER_INDEX, gn), true);
+	gn->mtime = ArchMTimeMember(Var(ARCHIVE_INDEX, gn),
+	     Var(MEMBER_INDEX, gn), true);
 
 	return gn->mtime;
 }
@@ -940,7 +940,7 @@ Arch_MemMTime(GNode *gn)
 void
 Arch_FindLib(GNode *gn, Lst path UNUSED)
 {
-	Varq_Set(TARGET_INDEX, gn->name, gn);
+	Var(TARGET_INDEX, gn) = gn->name;
 }
 
 /*-
diff --git a/usr.bin/make/engine.c b/usr.bin/make/engine.c
index 9c04a262377..d1a6eca4ac6 100644
--- a/usr.bin/make/engine.c
+++ b/usr.bin/make/engine.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: engine.c,v 1.15 2007/11/17 16:32:04 espie Exp $ */
+/*	$OpenBSD: engine.c,v 1.16 2007/11/17 16:39:45 espie Exp $ */
 /*
  * Copyright (c) 1988, 1989, 1990 The Regents of the University of California.
  * Copyright (c) 1988, 1989 by Adam de Boor
@@ -60,9 +60,9 @@
 #include "error.h"
 #include "str.h"
 #include "memory.h"
+#include "buf.h"
 
 static void MakeTimeStamp(void *, void *);
-static void MakeAddAllSrc(void *, void *);
 static int rewrite_time(const char *);
 static void setup_signal(int);
 static void setup_all_signals(void);
@@ -102,7 +102,7 @@ Job_CheckCommands(GNode *gn, void (*abortProc)(char *, ...))
 			 * attributes or sources attached to .DEFAULT itself.
 			 */
 			Make_HandleUse(DEFAULT, gn);
-			Varq_Set(IMPSRC_INDEX, Varq_Value(TARGET_INDEX, gn), gn);
+			Var(IMPSRC_INDEX, gn) = Var(TARGET_INDEX, gn);
 		} else if (is_out_of_date(Dir_MTime(gn))) {
 			/*
 			 * The node wasn't the target of an operator we have no
@@ -240,16 +240,25 @@ Make_HandleUse(GNode	*cgn,	/* The .USE node */
 	}
 }
 
-static void
-MakeAddAllSrc(void *cgnp, void *pgnp)
+void
+Make_DoAllVar(GNode *gn)
 {
-	GNode	*child = (GNode *)cgnp;
-	GNode	*parent = (GNode *)pgnp;
-	if ((child->type & (OP_EXEC|OP_USE|OP_INVISIBLE)) == 0) {
-		const char *target;
-
+	GNode *child;
+	LstNode ln;
+	BUFFER allsrc, oodate;
+	char *target;
+	bool do_oodate;
+	int oodate_count, allsrc_count = 0;
+
+	oodate_count = 0;
+	allsrc_count = 0;
+
+	for (ln = Lst_First(&gn->children); ln != NULL; ln = Lst_Adv(ln)) {
+		child = (GNode *)Lst_Datum(ln);
+		if ((child->type & (OP_EXEC|OP_USE|OP_INVISIBLE)) != 0)
+			continue;
 		if (OP_NOP(child->type) ||
-		    (target = Varq_Value(TARGET_INDEX, child)) == NULL) {
+		    (target = Var(TARGET_INDEX, child)) == NULL) {
 			/*
 			 * this node is only source; use the specific pathname
 			 * for it
@@ -258,46 +267,63 @@ MakeAddAllSrc(void *cgnp, void *pgnp)
 			    child->name;
 		}
 
-		Varq_Append(ALLSRC_INDEX, target, parent);
-		if (parent->type & OP_JOIN) {
+		/*
+		 * It goes in the OODATE variable if the parent is younger than
+		 * the child or if the child has been modified more recently
+		 * than the start of the make.  This is to keep make from
+		 * getting confused if something else updates the parent after
+		 * the make starts (shouldn't happen, I know, but sometimes it
+		 * does). In such a case, if we've updated the kid, the parent
+		 * is likely to have a modification time later than that of the
+		 * kid and anything that relies on the OODATE variable will be
+		 * hosed.
+		 */
+		do_oodate = false;
+		if (gn->type & OP_JOIN) {
 			if (child->built_status == MADE)
-				Varq_Append(OODATE_INDEX, target, parent);
-		} else if (is_strictly_before(parent->mtime, child->mtime) ||
+				do_oodate = true;
+		} else if (is_strictly_before(gn->mtime, child->mtime) ||
 		   (!is_strictly_before(child->mtime, now) &&
-		   child->built_status == MADE)) {
-			/*
-			 * It goes in the OODATE variable if the parent is
-			 * younger than the child or if the child has been
-			 * modified more recently than the start of the make.
-			 * This is to keep make from getting confused if
-			 * something else updates the parent after the
-			 * make starts (shouldn't happen, I know, but sometimes
-			 * it does). In such a case, if we've updated the kid,
-			 * the parent is likely to have a modification time
-			 * later than that of the kid and anything that relies
-			 * on the OODATE variable will be hosed.
-			 */
-			Varq_Append(OODATE_INDEX, target, parent);
+		   child->built_status == MADE))
+		   	do_oodate = true;
+		if (do_oodate) {
+			oodate_count++;
+			if (oodate_count == 1)
+				Var(OODATE_INDEX, gn) = target;
+			else {
+				if (oodate_count == 2) {
+					Buf_Init(&oodate, 0);
+					Buf_AddString(&oodate, 
+					    Var(OODATE_INDEX, gn));
+				}
+				Buf_AddSpace(&oodate);
+				Buf_AddString(&oodate, target);
+			}
+		}
+		allsrc_count++;
+		if (allsrc_count == 1)
+			Var(ALLSRC_INDEX, gn) = target;
+		else {
+			if (allsrc_count == 2) {
+				Buf_Init(&allsrc, 0);
+				Buf_AddString(&allsrc, 
+				    Var(ALLSRC_INDEX, gn));
+			}
+			Buf_AddSpace(&allsrc);
+			Buf_AddString(&allsrc, target);
 		}
 	}
-}
 
-void
-Make_DoAllVar(GNode *gn)
-{
-	Lst_ForEach(&gn->children, MakeAddAllSrc, gn);
+	if (allsrc_count > 1)
+		Var(ALLSRC_INDEX, gn) = Buf_Retrieve(&allsrc);
+	if (oodate_count > 1)
+		Var(OODATE_INDEX, gn) = Buf_Retrieve(&oodate);
 
 	if (gn->impliedsrc)
-		Varq_Set(IMPSRC_INDEX, 
-		    Varq_Value(TARGET_INDEX, gn->impliedsrc), gn);
+		Var(IMPSRC_INDEX, gn) = Var(TARGET_INDEX, gn->impliedsrc);
 			
-	if (Varq_Value(OODATE_INDEX, gn) == NULL)
-		Varq_Set(OODATE_INDEX, "", gn);
-	if (Varq_Value(ALLSRC_INDEX, gn) == NULL)
-		Varq_Set(ALLSRC_INDEX, "", gn);
-
 	if (gn->type & OP_JOIN)
-		Varq_Set(TARGET_INDEX, Varq_Value(ALLSRC_INDEX, gn), gn);
+		Var(TARGET_INDEX, gn) = Var(ALLSRC_INDEX, gn);
 }
 
 /* Wrapper to call Make_TimeStamp from a forEach loop.	*/
@@ -705,7 +731,7 @@ static void
 handle_compat_interrupts(GNode *gn)
 {
 	if (!Targ_Precious(gn)) {
-		char	  *file = Varq_Value(TARGET_INDEX, gn);
+		char *file = Var(TARGET_INDEX, gn);
 
 		if (!noExecute && eunlink(file) != -1)
 			Error("*** %s removed\n", file);
diff --git a/usr.bin/make/make.c b/usr.bin/make/make.c
index 243fd0c8006..c4634aafb79 100644
--- a/usr.bin/make/make.c
+++ b/usr.bin/make/make.c
@@ -1,5 +1,5 @@
 /*	$OpenPackages$ */
-/*	$OpenBSD: make.c,v 1.49 2007/11/10 13:59:48 espie Exp $	*/
+/*	$OpenBSD: make.c,v 1.50 2007/11/17 16:39:45 espie Exp $	*/
 /*	$NetBSD: make.c,v 1.10 1996/11/06 17:59:15 christos Exp $	*/
 
 /*
@@ -146,7 +146,7 @@ Make_Update(GNode *cgn)	/* the child node */
 	char	*cname; /* the child's name */
 	LstNode	ln;	/* Element in parents list */
 
-	cname = Varq_Value(TARGET_INDEX, cgn);
+	cname = Var(TARGET_INDEX, cgn);
 
 	/*
 	 * If the child was actually made, see what its modification time is
diff --git a/usr.bin/make/suff.c b/usr.bin/make/suff.c
index 37556951e88..206c63cfe18 100644
--- a/usr.bin/make/suff.c
+++ b/usr.bin/make/suff.c
@@ -1,5 +1,5 @@
 /*	$OpenPackages$ */
-/*	$OpenBSD: suff.c,v 1.75 2007/11/06 21:15:19 espie Exp $ */
+/*	$OpenBSD: suff.c,v 1.76 2007/11/17 16:39:45 espie Exp $ */
 /*	$NetBSD: suff.c,v 1.13 1996/11/06 17:59:25 christos Exp $	*/
 
 /*
@@ -1391,8 +1391,8 @@ SuffFindArchiveDeps(
 	}
 
 	/* Copy variables from member node to this one.  */
-	Varq_Set(TARGET_INDEX, Varq_Value(TARGET_INDEX, mem), gn);
-	Varq_Set(PREFIX_INDEX, Varq_Value(PREFIX_INDEX, mem), gn);
+	Var(TARGET_INDEX, gn) = Var(TARGET_INDEX, mem);
+	Var(PREFIX_INDEX, gn) = Var(PREFIX_INDEX, mem);
 
 	ms = mem->suffix;
 	if (ms == NULL) {
@@ -1405,8 +1405,8 @@ SuffFindArchiveDeps(
 
 
 	/* Set the other two local variables required for this target.  */
-	Varq_Set(MEMBER_INDEX, mem->name, gn);
-	Varq_Set(ARCHIVE_INDEX, gn->name, gn);
+	Var(MEMBER_INDEX, gn) = mem->name;
+	Var(ARCHIVE_INDEX, gn) = gn->name;
 
 	if (ms != NULL) {
 		/*
@@ -1601,10 +1601,10 @@ SuffFindNormalDeps(
 	 * since it's only set to the path if the thing is only a source and
 	 * if it's only a source, it doesn't matter what we put here as far
 	 * as expanding sources is concerned, since it has none...	*/
-	Varq_Set(TARGET_INDEX, gn->name, gn);
+	Var(TARGET_INDEX, gn) = gn->name;
 
-	pref = targ != NULL ? targ->pref : gn->name;
-	Varq_Set(PREFIX_INDEX, pref, gn);
+	pref = targ != NULL ? estrdup(targ->pref) : gn->name;
+	Var(PREFIX_INDEX, gn) = pref;
 
 	/* Now we've got the important local variables set, expand any sources
 	 * that still contain variables or wildcards in their names.  */
@@ -1629,7 +1629,7 @@ sfnd_abort:
 			    &targ->suff->searchPath));
 			if (gn->path != NULL) {
 				char *ptr;
-				Varq_Set(TARGET_INDEX, gn->path, gn);
+				Var(TARGET_INDEX, gn) = estrdup(gn->path);
 
 				if (targ != NULL) {
 					/* Suffix known for the thing -- trim
@@ -1650,7 +1650,7 @@ sfnd_abort:
 					else
 						ptr = gn->path;
 
-					Varq_Set(PREFIX_INDEX, ptr, gn);
+					Var(PREFIX_INDEX, gn) = estrdup(ptr);
 
 					gn->path[savep] = savec;
 				} else {
@@ -1664,7 +1664,7 @@ sfnd_abort:
 					else
 						ptr = gn->path;
 
-					Varq_Set(PREFIX_INDEX, ptr, gn);
+					Var(PREFIX_INDEX, gn) = estrdup(ptr);
 				}
 			}
 		} else {
@@ -1740,9 +1740,9 @@ sfnd_abort:
 			 * is set the standard and System V variables.  */
 			targ->node->type |= OP_DEPS_FOUND;
 
-			Varq_Set(PREFIX_INDEX, targ->pref, targ->node);
+			Var(PREFIX_INDEX, targ->node) = estrdup(targ->pref);
 
-			Varq_Set(TARGET_INDEX, targ->node->name, targ->node);
+			Var(TARGET_INDEX, targ->node) = targ->node->name;
 		}
 	}
 
@@ -1834,14 +1834,14 @@ SuffFindDeps(GNode *gn, Lst slst)
 			Arch_FindLib(gn, &s->searchPath);
 		} else {
 			gn->suffix = NULL;
-			Varq_Set(TARGET_INDEX, gn->name, gn);
+			Var(TARGET_INDEX, gn) = gn->name;
 		}
 		/*
 		 * Because a library (-lfoo) target doesn't follow the standard
 		 * filesystem conventions, we don't set the regular variables
 		 * for the thing. .PREFIX is simply made empty...
 		 */
-		Varq_Set(PREFIX_INDEX, "", gn);
+		Var(PREFIX_INDEX, gn) = "";
 	} else
 		SuffFindNormalDeps(gn, slst);
 }
diff --git a/usr.bin/make/symtable.h b/usr.bin/make/symtable.h
index 8b109160d94..368d035d5de 100644
--- a/usr.bin/make/symtable.h
+++ b/usr.bin/make/symtable.h
@@ -1,7 +1,7 @@
 #ifndef SYMTABLE_H
 #define SYMTABLE_H
 /*	$OpenPackages$ */
-/*	$OpenBSD: symtable.h,v 1.1 2001/05/23 12:34:50 espie Exp $ */
+/*	$OpenBSD: symtable.h,v 1.2 2007/11/17 16:39:45 espie Exp $ */
 
 /*
  * Copyright (c) 2001 Marc Espie.
@@ -28,15 +28,13 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-struct Var_;
-
 #define LOCAL_SIZE	7
 
 /* SymTable is private to var.c, but is declared here to allow for
    local declaration of context tables
  */
 struct SymTable_ {
-	struct Var_ *locals[LOCAL_SIZE];
+	char *locals[LOCAL_SIZE];
 };
 
 #endif
diff --git a/usr.bin/make/var.c b/usr.bin/make/var.c
index b161b41cae4..d31a34d492a 100644
--- a/usr.bin/make/var.c
+++ b/usr.bin/make/var.c
@@ -1,5 +1,5 @@
 /*	$OpenPackages$ */
-/*	$OpenBSD: var.c,v 1.82 2007/11/04 09:31:57 espie Exp $	*/
+/*	$OpenBSD: var.c,v 1.83 2007/11/17 16:39:45 espie Exp $	*/
 /*	$NetBSD: var.c,v 1.18 1997/03/18 19:24:46 christos Exp $	*/
 
 /*
@@ -205,7 +205,6 @@ static struct ohash_info var_info = {
 };
 
 static int classify_var(const char *, const char **, uint32_t *);
-static Var *find_any_var(const char *, const char *, SymTable *, int, uint32_t);
 static Var *find_global_var(const char *, const char *, uint32_t);
 static Var *find_global_var_without_env(const char *, const char *, uint32_t);
 static void fill_from_env(Var *);
@@ -215,7 +214,6 @@ static void var_set_value(Var *, const char *);
 #define var_get_value(v)	Buf_Retrieve(&((v)->val))
 static void var_append_value(Var *, const char *);
 static void poison_check(Var *);
-static void varq_set_append(int, const char *, GNode *, bool);
 static void var_set_append(const char *, const char *, const char *, int, bool);
 static void set_magic_shell_variable(void);
 
@@ -229,7 +227,8 @@ typedef const char * (*find_t)(const char *);
 static find_t find_pos(int);
 static void push_used(Var *);
 static void pop_used(Var *);
-static char *get_expanded_value(Var *, int, SymTable *, bool, bool *);
+static char *get_expanded_value(const char *, const char *, int, uint32_t,
+    SymTable *, bool, bool *);
 static bool parse_base_variable_name(const char **, struct Name *, SymTable *);
 
 
@@ -455,59 +454,6 @@ SymTable_Destroy(SymTable *ctxt)
 }
 #endif
 
-/* set or append to dynamic variable.
- */
-static void
-varq_set_append(int idx, const char *val, GNode *gn, bool append)
-{
-	Var *v = gn->context.locals[idx];
-
-	if (v == NULL) {
-		v = create_var(varnames[idx], NULL);
-#ifdef STATS_VAR_LOOKUP
-		STAT_VAR_CREATION++;
-#endif
-		if (val != NULL)
-			var_set_initial_value(v, val);
-		else
-			Buf_Init(&(v->val), 1);
-		v->flags = 0;
-		gn->context.locals[idx] = v;
-	} else {
-		if (append)
-			Buf_AddSpace(&(v->val));
-		else
-			Buf_Reset(&(v->val));
-		Buf_AddString(&(v->val), val);
-	}
-	if (DEBUG(VAR))
-		printf("%s:%s = %s\n", gn->name, varnames[idx],
-		    var_get_value(v));
-}
-
-void
-Varq_Set(int idx, const char *val, GNode *gn)
-{
-	varq_set_append(idx, val, gn, false);
-}
-
-void
-Varq_Append(int idx, const char *val, GNode *gn)
-{
-	varq_set_append(idx, val, gn, true);
-}
-
-char *
-Varq_Value(int idx, GNode *gn)
-{
-	Var *v = gn->context.locals[idx];
-
-	if (v == NULL)
-		return NULL;
-	else
-		return var_get_value(v);
-}
-
 /***
  ***	Global variable handling.
  ***/
@@ -766,26 +712,6 @@ Var_Definedi(const char *name, const char *ename)
  ***/
 
 
-/* XXX contrary to find_global_var(), find_any_var() can return NULL pointers.
- */
-static Var *
-find_any_var(const char *name, const char *ename, SymTable *ctxt,
-    int idx, uint32_t k)
-{
-	/* Handle local variables first */
-	if (idx != GLOBAL_INDEX) {
-		if (ctxt != NULL) {
-			if (idx < LOCAL_SIZE)
-				return ctxt->locals[idx];
-			else
-				return ctxt->locals[EXTENDED2SIMPLE(idx)];
-		} else
-			return NULL;
-	} else {
-		return find_global_var(name, ename, k);
-	}
-}
-
 /* All the scanning functions needed to account for all the forms of
  * variable names that exist:
  *	$A, ${AB}, $(ABC), ${A:mod}, $(A:mod)
@@ -929,18 +855,11 @@ pop_used(Var *v)
 }
 
 static char *
-get_expanded_value(Var *v, int idx, SymTable *ctxt, bool err, bool *freePtr)
+get_expanded_value(const char *name, const char *ename, int idx, uint32_t k, 
+    SymTable *ctxt, bool err, bool *freePtr)
 {
 	char *val;
 
-	if (v == NULL)
-		return NULL;
-
-	if ((v->flags & POISONS) != 0)
-		poison_check(v);
-	if ((v->flags & VAR_DUMMY) != 0)
-		return NULL;
-
 	/* Before doing any modification, we have to make sure the
 	 * value has been fully expanded. If it looks like recursion
 	 * might be necessary (there's a dollar sign somewhere in
@@ -949,20 +868,42 @@ get_expanded_value(Var *v, int idx, SymTable *ctxt, bool err, bool *freePtr)
 	 * value returned by Var_Subst will have been dynamically
 	 * allocated, so it will need freeing when we return.
 	 */
-	val = var_get_value(v);
 	if (idx == GLOBAL_INDEX) {
+		Var *v = find_global_var(name, ename, k);
+
+		if (v == NULL)
+			return NULL;
+
+		if ((v->flags & POISONS) != 0)
+			poison_check(v);
+		if ((v->flags & VAR_DUMMY) != 0)
+			return NULL;
+
+		val = var_get_value(v);
 		if (strchr(val, '$') != NULL) {
 			push_used(v);
 			val = Var_Subst(val, ctxt, err);
 			pop_used(v);
 			*freePtr = true;
 		}
-	} else if (idx >= LOCAL_SIZE) {
-		if (IS_EXTENDED_F(idx))
-			val = Var_GetTail(val);
-		else
-			val = Var_GetHead(val);
-		*freePtr = true;
+	} else {
+		if (ctxt != NULL) {
+			if (idx < LOCAL_SIZE)
+				val = ctxt->locals[idx];
+			else
+				val = ctxt->locals[EXTENDED2SIMPLE(idx)];
+		} else
+			val = NULL;
+		if (val == NULL)
+			return NULL;
+
+		if (idx >= LOCAL_SIZE) {
+			if (IS_EXTENDED_F(idx))
+				val = Var_GetTail(val);
+			else
+				val = Var_GetHead(val);
+			*freePtr = true;
+		}
 	}
 	return val;
 }
@@ -975,7 +916,6 @@ Var_Parse(const char *str,	/* The string to parse */
     bool *freePtr)		/* OUT: true if caller should free result */
 {
 	const char *tstr;
-	Var *v;
 	struct Name name;
 	char *val;
 	uint32_t k;
@@ -989,8 +929,7 @@ Var_Parse(const char *str,	/* The string to parse */
 	has_modifier = parse_base_variable_name(&tstr, &name, ctxt);
 
 	idx = classify_var(name.s, &name.e, &k);
-	v = find_any_var(name.s, name.e, ctxt, idx, k);
-	val = get_expanded_value(v, idx, ctxt, err, freePtr);
+	val = get_expanded_value(name.s, name.e, idx, k, ctxt, err, freePtr);
 	if (has_modifier) {
 		val = VarModifiers_Apply(val, &name, ctxt, err, freePtr,
 		    &tstr, str[1]);
diff --git a/usr.bin/make/var.h b/usr.bin/make/var.h
index 4cd4ffecba6..f2cf06c59fa 100644
--- a/usr.bin/make/var.h
+++ b/usr.bin/make/var.h
@@ -71,15 +71,9 @@ extern void Var_Deletei(const char *, const char *);
 #define OODATE_INDEX	4
 #define ALLSRC_INDEX	5
 #define IMPSRC_INDEX	6
-/* value = Varq_Value(index, node);
- *	Returns value of dynamic variable for a given node. */
-extern char *Varq_Value(int,  GNode *);
-/* Varq_Set(index, val, node);
- *	Sets value of dynamic variable for a given node. Copies val. */
-extern void Varq_Set(int, const char *, GNode *);
-/* Varq_Append(index, val, node);
- *	Appends to value of dynamic variable for a given node. */
-extern void Varq_Append(int, const char *, GNode *);
+
+#define Var(idx, gn)	((gn)->context.locals[idx])
+
 
 /* SymTable_Init(t);
  *	Inits the local symtable in a GNode. */