Project

General

Profile

Actions

Bug #13631

closed

shell lint about variable expansion in arithmetic expressions is inscrutable

Added by Robert Mustacchi 9 months ago. Updated 8 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
cmd - userland programs
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

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.


Related issues

Related to illumos gate - Bug #13601: ksh shell lint misleadingClosedAndy Fiddaman

Actions
Actions #1

Updated by Robert Mustacchi 9 months ago

  • Related to Bug #13601: ksh shell lint misleading added
Actions #2

Updated by Andy Fiddaman 9 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.
Actions #3

Updated by Robert Mustacchi 9 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.

Actions #4

Updated by Andy Fiddaman 8 months ago

  • Status changed from New to In Progress
  • Assignee set to Andy Fiddaman
Actions #5

Updated by Electric Monk 8 months ago

  • Gerrit CR set to 1384
Actions #6

Updated by Andy Fiddaman 8 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

to

/data/omnios-build/ksh/linttest: warning: line 12: in '(( $a > 1 ))', using '$' is unnecessary, incurs a penalty and can introduce rounding errors.
Actions #7

Updated by Electric Monk 8 months ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit c7b656fc7125adc5888ce934f1a3433da4b1bbc5

commit  c7b656fc7125adc5888ce934f1a3433da4b1bbc5
Author: Andy Fiddaman <omnios@citrus-it.co.uk>
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 <rm@fingolfin.org>
    Approved by: Dan McDonald <danmcd@joyent.com>

Actions

Also available in: Atom PDF