Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/usr.bin/make make: clean up memory management in evaluation ...
details: https://anonhg.NetBSD.org/src/rev/14f3a582ad28
branches: trunk
changeset: 959461:14f3a582ad28
user: rillig <rillig%NetBSD.org@localhost>
date: Sun Feb 14 22:48:17 2021 +0000
description:
make: clean up memory management in evaluation of expressions
The condition "st->newValue.str != val" in ApplySingleModifier made the
memory management look more complicated than it really was. Freeing an
object based on another object's value is harder to understand than
necessary.
To fix this, the "current value" of the expression is now stored in
ApplyModifiersState, and it gets updated in-place by the ApplyModifier
functions. This reduces the number of parameters for the ApplyModifier
functions.
Accessing the current value of the expression is now more verbose than
before (st->value.str instead of the simple val). To compensate for
this verbosity, ApplyModifiersIndirect is now much easier to understand
since there is no extra "current value" floating around.
There is still room for improvement. In ApplyModifiers, passing an FStr
in and returning another (or possibly the same) makes it difficult to
understand memory management. Adding a separate Expr type that outlives
the ApplyModifiersState will make this easier, in a follow-up commit.
diffstat:
usr.bin/make/var.c | 264 ++++++++++++++++++++++++----------------------------
1 files changed, 123 insertions(+), 141 deletions(-)
diffs (truncated from 808 to 300 lines):
diff -r a49193243d49 -r 14f3a582ad28 usr.bin/make/var.c
--- a/usr.bin/make/var.c Sun Feb 14 21:54:42 2021 +0000
+++ b/usr.bin/make/var.c Sun Feb 14 22:48:17 2021 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: var.c,v 1.820 2021/02/14 21:54:42 rillig Exp $ */
+/* $NetBSD: var.c,v 1.821 2021/02/14 22:48:17 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990, 1993
@@ -139,7 +139,7 @@
#include "metachar.h"
/* "@(#)var.c 8.3 (Berkeley) 3/19/94" */
-MAKE_RCSID("$NetBSD: var.c,v 1.820 2021/02/14 21:54:42 rillig Exp $");
+MAKE_RCSID("$NetBSD: var.c,v 1.821 2021/02/14 22:48:17 rillig Exp $");
typedef enum VarFlags {
VAR_NONE = 0,
@@ -1934,9 +1934,9 @@
/*
* The ApplyModifier functions take an expression that is being evaluated.
* Their task is to apply a single modifier to the expression.
- * To do this, they parse the modifier and its parameters from pp and apply
- * the parsed modifier to the current value of the expression, generating a
- * new value from it.
+ * To do this, they parse the modifier and its parameters from pp, apply
+ * the parsed modifier to the current value of the expression and finally
+ * update the value of the expression.
*
* The modifier typically lasts until the next ':', or a closing '}' or ')'
* (taken from st->endc), or the end of the string (parse error).
@@ -1983,8 +1983,9 @@
* during parsing though.
*
* Evaluating the modifier usually takes the current value of the variable
- * expression from st->val, or the variable name from st->var->name and stores
- * the result in st->newValue.
+ * expression from st->value, or the variable name from st->var->name and
+ * stores the result back in st->value via Expr_SetValueOwn or
+ * Expr_SetValueRefer.
*
* If evaluating fails (as of 2020-08-23), an error message is printed using
* Error. This function has no side-effects, it really just prints the error
@@ -1995,7 +1996,7 @@
* Housekeeping
*
* Some modifiers such as :D and :U turn undefined expressions into defined
- * expressions (see VEF_UNDEF, VEF_DEF).
+ * expressions (see Expr_Define).
*
* Some modifiers need to free some memory.
*/
@@ -2027,11 +2028,8 @@
Var *const var;
GNode *const scope;
const VarEvalFlags eflags;
- /*
- * The new value of the expression, after applying the modifier,
- * never NULL.
- */
- FStr newValue;
+ /* The value of the expression, never NULL. */
+ FStr value;
/* Word separator in expansions (see the :ts modifier). */
char sep;
/*
@@ -2055,13 +2053,15 @@
static void
Expr_SetValueOwn(Expr *expr, char *value)
{
- expr->newValue = FStr_InitOwn(value);
+ FStr_Done(&expr->value);
+ expr->value = FStr_InitOwn(value);
}
static void
Expr_SetValueRefer(Expr *expr, const char *value)
{
- expr->newValue = FStr_InitRefer(value);
+ FStr_Done(&expr->value);
+ expr->value = FStr_InitRefer(value);
}
typedef enum ApplyModifierResult {
@@ -2332,7 +2332,7 @@
* modifyWord_args Custom arguments for modifyWord
*/
static void
-ModifyWords(ApplyModifiersState *st, const char *str,
+ModifyWords(ApplyModifiersState *st,
ModifyWordsCallback modifyWord, void *modifyWord_args,
Boolean oneBigWord)
{
@@ -2342,16 +2342,16 @@
if (oneBigWord) {
SepBuf_Init(&result, st->sep);
- modifyWord(str, &result, modifyWord_args);
+ modifyWord(st->value.str, &result, modifyWord_args);
goto done;
}
SepBuf_Init(&result, st->sep);
- words = Str_Words(str, FALSE);
+ words = Str_Words(st->value.str, FALSE);
DEBUG2(VAR, "ModifyWords: split \"%s\" into %u words\n",
- str, (unsigned)words.len);
+ st->value.str, (unsigned)words.len);
for (i = 0; i < words.len; i++) {
modifyWord(words.words[i], &result, modifyWord_args);
@@ -2367,7 +2367,7 @@
/* :@var@...${var}...@ */
static ApplyModifierResult
-ApplyModifier_Loop(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Loop(const char **pp, ApplyModifiersState *st)
{
struct ModifyWord_LoopArgs args;
char prev_sep;
@@ -2394,7 +2394,7 @@
args.eflags = st->eflags & ~(unsigned)VARE_KEEP_DOLLAR;
prev_sep = st->sep;
st->sep = ' '; /* XXX: should be st->sep for consistency */
- ModifyWords(st, val, ModifyWord_Loop, &args, st->oneBigWord);
+ ModifyWords(st, ModifyWord_Loop, &args, st->oneBigWord);
st->sep = prev_sep;
/* XXX: Consider restoring the previous variable instead of deleting. */
/*
@@ -2409,7 +2409,7 @@
/* :Ddefined or :Uundefined */
static ApplyModifierResult
-ApplyModifier_Defined(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Defined(const char **pp, ApplyModifiersState *st)
{
Buffer buf;
const char *p;
@@ -2457,12 +2457,11 @@
Expr_Define(st);
- if (eflags & VARE_WANTRES) {
+ if (eflags & VARE_WANTRES)
Expr_SetValueOwn(st, Buf_DoneData(&buf));
- } else {
- Expr_SetValueRefer(st, val);
+ else
Buf_Done(&buf);
- }
+
return AMR_OK;
}
@@ -2497,7 +2496,7 @@
/* :gmtime */
static ApplyModifierResult
-ApplyModifier_Gmtime(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Gmtime(const char **pp, ApplyModifiersState *st)
{
time_t utc;
@@ -2517,14 +2516,13 @@
utc = 0;
*pp = mod + 6;
}
- Expr_SetValueOwn(st, VarStrftime(val, TRUE, utc));
+ Expr_SetValueOwn(st, VarStrftime(st->value.str, TRUE, utc));
return AMR_OK;
}
/* :localtime */
static ApplyModifierResult
-ApplyModifier_Localtime(const char **pp, const char *val,
- ApplyModifiersState *st)
+ApplyModifier_Localtime(const char **pp, ApplyModifiersState *st)
{
time_t utc;
@@ -2544,18 +2542,18 @@
utc = 0;
*pp = mod + 9;
}
- Expr_SetValueOwn(st, VarStrftime(val, FALSE, utc));
+ Expr_SetValueOwn(st, VarStrftime(st->value.str, FALSE, utc));
return AMR_OK;
}
/* :hash */
static ApplyModifierResult
-ApplyModifier_Hash(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Hash(const char **pp, ApplyModifiersState *st)
{
if (!ModMatch(*pp, "hash", st->endc))
return AMR_UNKNOWN;
- Expr_SetValueOwn(st, VarHash(val));
+ Expr_SetValueOwn(st, VarHash(st->value.str));
*pp += 4;
return AMR_OK;
}
@@ -2617,7 +2615,7 @@
* The :range=7 modifier generates an integer sequence from 1 to 7.
*/
static ApplyModifierResult
-ApplyModifier_Range(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Range(const char **pp, ApplyModifiersState *st)
{
size_t n;
Buffer buf;
@@ -2641,7 +2639,7 @@
}
if (n == 0) {
- Words words = Str_Words(val, FALSE);
+ Words words = Str_Words(st->value.str, FALSE);
n = words.len;
Words_Free(words);
}
@@ -2662,7 +2660,7 @@
/* :Mpattern or :Npattern */
static ApplyModifierResult
-ApplyModifier_Match(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Match(const char **pp, ApplyModifiersState *st)
{
const char *mod = *pp;
Boolean copy = FALSE; /* pattern should be, or has been, copied */
@@ -2730,17 +2728,17 @@
}
DEBUG3(VAR, "Pattern[%s] for [%s] is [%s]\n",
- st->var->name.str, val, pattern);
+ st->var->name.str, st->value.str, pattern);
callback = mod[0] == 'M' ? ModifyWord_Match : ModifyWord_NoMatch;
- ModifyWords(st, val, callback, pattern, st->oneBigWord);
+ ModifyWords(st, callback, pattern, st->oneBigWord);
free(pattern);
return AMR_OK;
}
/* :S,from,to, */
static ApplyModifierResult
-ApplyModifier_Subst(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Subst(const char **pp, ApplyModifiersState *st)
{
struct ModifyWord_SubstArgs args;
char *lhs, *rhs;
@@ -2792,7 +2790,7 @@
break;
}
- ModifyWords(st, val, ModifyWord_Subst, &args, oneBigWord);
+ ModifyWords(st, ModifyWord_Subst, &args, oneBigWord);
free(lhs);
free(rhs);
@@ -2803,7 +2801,7 @@
/* :C,from,to, */
static ApplyModifierResult
-ApplyModifier_Regex(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Regex(const char **pp, ApplyModifiersState *st)
{
char *re;
struct ModifyWord_SubstRegexArgs args;
@@ -2856,7 +2854,7 @@
if (args.nsub > 10)
args.nsub = 10;
- ModifyWords(st, val, ModifyWord_SubstRegex, &args, oneBigWord);
+ ModifyWords(st, ModifyWord_SubstRegex, &args, oneBigWord);
regfree(&args.re);
free(args.replace);
@@ -2867,10 +2865,10 @@
/* :Q, :q */
static ApplyModifierResult
-ApplyModifier_Quote(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_Quote(const char **pp, ApplyModifiersState *st)
{
if ((*pp)[1] == st->endc || (*pp)[1] == ':') {
- Expr_SetValueOwn(st, VarQuote(val, **pp == 'q'));
+ Expr_SetValueOwn(st, VarQuote(st->value.str, **pp == 'q'));
(*pp)++;
return AMR_OK;
} else
@@ -2886,7 +2884,7 @@
/* :ts<separator> */
static ApplyModifierResult
-ApplyModifier_ToSep(const char **pp, const char *val, ApplyModifiersState *st)
+ApplyModifier_ToSep(const char **pp, ApplyModifiersState *st)
{
const char *sep = *pp + 2;
Home |
Main Index |
Thread Index |
Old Index