Re: zmathfunc: min, max, sum throw error if result equals 0

["Daniel Shahaf" <d.s@xxxxxxxxxxxxxxxxxx>]

Bart Schaefer wrote on Sun, 07 Mar 2021 21:39 +00:00:
> On Sun, Mar 7, 2021 at 9:17 AM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Nikolaus Thiel wrote on Sun, Mar 07, 2021 at 17:37:09 +0100:
> > >
> > > When the result of min, max or sum equals 0, the functions throw an error.
> 
> I would not equate "return nozero" with "throw an error", FWIW.
> 
> > Ouch.  Patch series attached.
> 
> I think your regression tests cover this, but use of "true" avoids
> changing the result in "functions -M" context, correct?

Yeah.  «return 0» causes the arithmetic expression to evaluate to 0,
because the argument to «return» is math-evaluated.

> Is it worth testing invalid cases?  Such as uses outside math context
> where the arguments are not syntax checked?

I'd say this specific example of an invalid case is lower priority,
since there's no promise of any particular failure mode.  We could add
a test to ensure it doesn't format C:, of course.

A test for min() and max() with zero arguments would make sense, since
those cases actually do promise a specific failure mode.

Could also add tests with three arguments in various permutations.

If you have ideas, feel free to write them and post them; I'll
transplant them into Z02 once I have committed it.

> As long as we're on the subject:
> 
> zsh_math_func_min() {
>   local result=$1
>   shift
>   local arg
>   for arg ; do
>     (( $arg < result )) && result=$arg
>   done
>   (( result ))
>   true # Careful here: `return 0` evaluates an arithmetic expression
> }
> 
> Because of the way math context works, if any of $@ is a string that
> can be interpreted as a math expression, the above will evaluate it at
> least twice (and up to $# times in the case of $1).  This could have
> side-effects.

Could you post a regression test for this?

Related to code as it is in master, are «(( $arg < result ))» and
«(( arg < result ))» equivalent?

> zsh_math_func_min() {
>   local result=$(( $1 ))

Doesn't this still do multiple string-to-number conversions?

>   shift
>   local arg n
>   for arg ; do
>     (( n = arg )) # evaluate arg exactly once
>     (( n < result && (result = n) ))
>   done
>   (( result ))
>   true # Careful here: `return 0` evaluates an arithmetic expression
> }

Hmm.  I'll go ahead and push the series now, then, so it'll be easier
for you to work on revisions.  If anyone has commit reviews, don't let
the fact that I've pushed things affect you.

Cheers,

Daniel