shell lint about variable expansion in arithmetic expressions is inscrutable
I had a bit of code that pbchk was upset about:
if (( $pcidb_exit == 0 )); then printf "All tests passed successfully!\n" fi
pbchk said: " line 219: variable expansion makes arithmetic evaluation less efficient"
The problem with this message is that you need to realize that in arithmetic evaluation context that you can actually reference variable names directly without the
$ and that including it changes things around. If you're not familiar with this error message, this is mostly inscrutable. For example, the native translation from the other pbchk error message about writing this as
if [[ $pcidb_exit -eq 0 ]] will tell you to use arithmetic expressions instead. It'd really help to have these pbchk error messages be something that explain what's wrong for folks who aren't as familiar with all the intricacies of the shell especially as they're being told to rewrite things from a form that is certainly correct to another form to appease pbchk.
Updated by Andy Fiddaman 2 months ago
I wonder if we can just pad out the lint output to be more informative, or whether to be more selective about what warnings to print. I already suppressed the one about grave accents being deprecated.
This one does seem a worthwhile warning, just hard to interpret - the shell docbook that's still in the gate tree says:
Avoid string to number and/or number to string conversions in arithmetic expressions to avoid performance degradation and rounding errors. Example 6. (( x=$x*2 )) vs. (( x=x*2 )) float x ... (( x=$x*2 )) will convert the variable "x" (stored in the machine's native |long double| datatype) to a string value in base10 format, apply pattern expansion (globbing), then insert this string into the arithmetic expressions and parse the value which converts it into the internal |long double| datatype format again. This is both slow and generates rounding errors when converting the floating-point value between the internal base2 and the base10 representation of the string. The correct usage would be: float x ... (( x=x*2 )) e.g. omit the '$' because it's (at least) redundant within arithmetic expressions.
Updated by Robert Mustacchi 2 months ago
I wasn't asking to get rid of the warning, mainly to make sure when we have this kind of warning to explain what's going on in more detail. The reason this isn't immediately obvious is that special variables (like the case where I hit this in #13601 require you to still use a leading
$. I think error messages when there's additional context about how to correctly fix it will help folks out a lot.
Updated by Andy Fiddaman about 2 months ago
With the change in the associated Gerrit review, this changes from:
/data/omnios-build/ksh/linttest: warning: line 12: variable expansion makes arithmetic evaluation less efficient
/data/omnios-build/ksh/linttest: warning: line 12: in '(( $a > 1 ))', using '$' is unnecessary, incurs a penalty and can introduce rounding errors.
Updated by Electric Monk about 1 month ago
- Status changed from In Progress to Closed
- % Done changed from 0 to 100
commit c7b656fc7125adc5888ce934f1a3433da4b1bbc5 Author: Andy Fiddaman <email@example.com> Date: 2021-04-07T19:16:33.000Z 13601 ksh shell lint misleading 13631 shell lint about variable expansion in arithmetic expressions is inscrutable Reviewed by: Robert Mustacchi <firstname.lastname@example.org> Approved by: Dan McDonald <email@example.com>