This also changes the conditions to their positive form, which is easier
to read.
No functional change. The resulting binary would have been the same as
before, were it not for the changed line numbers in the lint_assert
calls further down in the code.
Internally the code confuses the concept of "the user doesn't want
a backup file" and "the user hasn't defined a type of backup file".
Introduce a new "undefined" backup type to serve the purpose "none"
previously did, and make "none" not generate backup files, as expected.
http://mail-index.netbsd.org/tech-userlevel/2021/02/19/msg012901.html
XXX pullup?
When the point that "isn't going to end well" is reached, the stacktrace
is quite long but still reasonable:
main
main_ReadFiles
ReadAllMakefiles
ReadMakefile directive-export.mk
Parse_File
ParseLine # line 3 has: _!= :;:
ParseVarassign
Parse_DoVar
VarAssign_Eval
VarAssign_EvalShell # because of the '!='
Cmd_Exec :;:
Var_ReexportVars # before starting the subprocess
ExportVar EMPTY_SHELL
ExportVarEnv # was only marked for export
Var_Subst ${EMPTY_SHELL} # to get the value to export
VarSubstExpr ${EMPTY_SHELL}
Var_Parse ${EMPTY_SHELL}
Var_Subst ${:sh} # since EMPTY_SHELL= ${:sh}
VarSubstExpr ${:sh}
Var_Parse ${:sh}
ApplyModifiers
ApplySingleModifier :sh
ApplyModifier
ApplyModifier_SunShell :sh
Cmd_Exec "" # empty command
Var_ReexportVars
ExportVar EMPTY_SHELL
ExportVarEnv # skipping this edge case
During the refactorings of the last months, several comments have become
outdated, some are now redundant since the code is as clear as the
comment, and some code benefits from a bit of explanation.
These are easier to read than hex constants.
There was no need to skip bits 2 and 3 (there were no constants for 0x04
and 0x08). Close this gap, to avoid confusing future readers. Keep the
relative order of the flags since that affects the debug output of -dv.
No functional change.
There are only very few places in var.c that contain really duplicate
code anymore.
There is still lots of _almost_ duplicate, for example the code for
parsing variable modifiers. It differs subtly in behavior:
* The modifiers ':M' and ':N' use '$$' to escape a '$' sign, while
almost all other modifiers use '\$' for this purpose.
* The modifiers ':M', ':N', ':S', ':@' and several others parse
balanced parentheses and braces, allowing '(' to '}' to match.
The modifiers ':D' and ':U' only treat the end character special but
not the other 3 of '(){}'.
* When parsing the modifier ':S' but not evaluating it, the code for
nested variable expressions is parsed differently from when it is in
evaluation mode (VARE_WANTRES). This applies to an outer ':S'
modifier and an inner ':D' or ':M' modifier.
Since these inconsistencies affect the behavior in edge cases and some
users of make might depend on it, they cannot be fixed by
behavior-preserving refactorings.
The type describes the definedness of an expression, not a general
status, therefore the new name is more precise.
The constants are renamed as well since their prefix 'VES' does not
match the type name anymore, it was correct 3 days ago when the type was
still named VarExprStatus. The name VES_NONE was misleading since
'none' does not describe its actual effect. That name came from the
time when the status was a bit set, and 'none' simply meant 'none of the
bits are set'.
The names used in debug logging will be renamed in a follow-up commit,
to demonstrate that the changes in this commit indeed have no functional
change, especially not the change from '!=' to '==' in line 4304.
No functional change.
The details of how variable expressions are evaluated is controlled by
several parameters: startc and endc differ for $(VAR) and ${VAR}, the
value of the expression can be interpreted as a single big word, and
when joining several words (such as with ':M' or ':S'), there may be a
custom word separator (defined with ':ts*').
The scope of half of these parameters is the whole variable expression,
the other half of the parameters are reset after each chain of indirect
modifiers. To make this distinction obvious in the code, extract Expr
from ApplyModifiersState. Previously, these details were hidden in how
parameters are passed and restored among ApplyModifiersIndirect and
ApplyModifiers.
The changes in the individual ApplyModifier functions are numerous but
straight-forward. They mostly replace 'st' with 'expr'.
The changes in ApplyModifiers and ApplyModifiersIndirect are more
subtle. The value of the expression is no longer passed around but is
stored in a fixed location, in Expr, which makes it easier to reason
about memory management.
The code in ApplyModifiers after 'cleanup' looks quite different but
preserves the existing behavior. Expr_SetValueRefer is nothing else
than the combination of FStr_Done followed by FStr_InitRefer. Storing
exprStatus back at the end was responsible for passing the definedness
of the expression after applying the indirect modifiers back to the
outer ApplyModifiersState. The same effect is now achieved by having
Expr.status with a wider scope.
No functional change.
That comment was useful when there was no function is_null_pointer.
Back then, the code for testing a null pointer was written in-line,
which made it really hard to see what's going on. This is no longer the
case.
In ModifyWords, there is no "passed string" anymore since that function
now directly operates on the expression value.
While here, improve the documentation of ModifyWordsCallback and rename
it to ModifyWordProc, focusing on its purpose instead of where it is
used.
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.
Memory management of the value of variable expressions is currently more
complicated than necessary. It is the responsibility of ApplyModifiers,
even though conceptually the value belongs to an expression, so it
should rather be in Expr. Right now, this is an alias for
ApplyModifiersState, but that will change soon.
When that is done, there will no longer be a "current value" and a "new
value", only a single "value" of an expression. At that point, before
Expr_SetValueOwn will overwrite the old value with the output of the
shell command, the error message needs to refer to the latter.
Single-character short variable expressions such as $V neither have a
starting character nor an ending character. The only interesting
character forms the complete variable name.
No functional change.
