From 81dd4a45363d38ac748c508ea1fa6f886a7c410d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 17 Dec 2017 12:31:39 -0800 Subject: [PATCH] [math] Remove more bare variable support Prior to this fix, a "bare variable" in math like 'x + 1' would be looked up in the environment, i.e. equivalent to '$x + 1'. This appears to have been done for performance. However this breaks the orthogonality of fish; performance is not a sufficient justification to give math this level of built-in power, especially because the performance of math is not a bottleneck. The implementation is also ugly. Remove this feature so that variables must be prefixed with the dollar sign and undergo normal variable expansion. Reading 'git grep' output does not show any uses of this in fish functions or completions. Also added to changelog. Fixes #4393 --- CHANGELOG.md | 1 + src/builtin_math.cpp | 49 -------------------------------------------- tests/math.err | 2 +- tests/math.in | 10 ++++----- tests/math.out | 2 +- 5 files changed, 8 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e692a8ef0..e21372ff2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ This section is for changes merged to the `major` branch that are not also merge - `bind` has a new `--silent` option to ignore bind requests for named keys not available under the current `$TERMINAL` (#4188, #4431) - `read` writes directly to stdout if called without arguments (#4407) - Globs are faster (#4579) +- "Bare variables" in math are no longer accepted. math uses normal variable expansion. (#4393) ## Other significant changes - Command substitution output is now limited to 10 MB by default (#3822). diff --git a/src/builtin_math.cpp b/src/builtin_math.cpp index 696749580..8e733b1cb 100644 --- a/src/builtin_math.cpp +++ b/src/builtin_math.cpp @@ -115,52 +115,6 @@ static const wchar_t *math_get_arg(int *argidx, wchar_t **argv, wcstring *storag return math_get_arg_argv(argidx, argv); } -// The MuParser mechanism for dynamic lookup of variables requires that we return a unique address -// for each variable. The following limit is arbitrary but anyone writing a math expression in fish -// that references more than one hundred unique variables is abusing fish. -#define MAX_RESULTS 100 -static double double_results[MAX_RESULTS]; -static int next_result = 0; - -/// Return a fish var converted to a double. This allows the user to use a bar var name in the -/// expression. That is `math a + 1` rather than `math $a + 1`. -static double *retrieve_var(const wchar_t *var_name, void *user_data) { - UNUSED(user_data); - static double zero_result = 0.0; - - auto var = env_get(var_name, ENV_DEFAULT); - if (!var) { - // We could report an error but we normally don't treat missing vars as a fatal error. - // throw mu::ParserError(L"Var '%ls' does not exist."); - return &zero_result; - } - if (var->empty()) { - return &zero_result; - } - - const wchar_t *first_val = var->as_list()[0].c_str(); - wchar_t *endptr; - errno = 0; - double result = wcstod(first_val, &endptr); - if (*endptr != L'\0' || errno) { - wchar_t errmsg[500]; - swprintf(errmsg, sizeof(errmsg) / sizeof(wchar_t), - _(L"Var '%ls' not a valid floating point number: '%ls'."), var_name, first_val); - throw mu::ParserError(errmsg); - } - - // We need to return a unique address for the var. If we used a `static double` var and returned - // it's address then multiple vars in the expression would all refer to the same value. - if (next_result == MAX_RESULTS - 1) { - wchar_t errmsg[500]; - swprintf(errmsg, sizeof(errmsg) / sizeof(wchar_t), - _(L"More than %d var names in math expression."), MAX_RESULTS); - throw mu::ParserError(errmsg); - } - double_results[next_result++] = result; - return double_results + next_result - 1; -} - /// Implement integer modulo math operator. static double moduloOperator(double v, double w) { return (int)v % std::max(1, (int)w); }; @@ -168,12 +122,9 @@ static double moduloOperator(double v, double w) { return (int)v % std::max(1, ( static int evaluate_expression(wchar_t *cmd, parser_t &parser, io_streams_t &streams, math_cmd_opts_t &opts, wcstring &expression) { UNUSED(parser); - next_result = 0; try { mu::Parser p; - // Setup callback so variables can be retrieved dynamically. - p.SetVarFactory(retrieve_var, nullptr); // MuParser doesn't implement the modulo operator so we add it ourselves since there are // likely users of our old math wrapper around bc that expect it to be available. p.DefineOprtChars(L"%"); diff --git a/tests/math.err b/tests/math.err index 240044f45..5a7ab5130 100644 --- a/tests/math.err +++ b/tests/math.err @@ -3,4 +3,4 @@ # Validate basic expressions #################### -# Validate how bare variables in an epxression are handled +# Validate how variables in an expression are handled diff --git a/tests/math.in b/tests/math.in index 8fee37df4..1e13114d7 100644 --- a/tests/math.in +++ b/tests/math.in @@ -14,11 +14,11 @@ math 5 \* -2 math -- -4 / 2 math -- '-4 * 2' -logmsg Validate how bare variables in an epxression are handled -math x + 1 +logmsg Validate how variables in an expression are handled +math $x + 1 set x 1 -math x + 1 +math $x + 1 set x 3 set y 1.5 -math '-x * y' -math -s1 '-x * y' +math "-$x * $y" +math -s1 "-$x * $y" diff --git a/tests/math.out b/tests/math.out index f0a91e8ce..df8607f71 100644 --- a/tests/math.out +++ b/tests/math.out @@ -17,7 +17,7 @@ -8 #################### -# Validate how bare variables in an epxression are handled +# Validate how variables in an expression are handled 1 2 -4