jimav / Spreadsheet-Edit

Perl Spreadsheet::Edit - comprehensive csv/spreadsheet slice & dice tool
1 stars 0 forks source link

BBC: Bleadperl v5.41.4 breaks Spreadsheet-Edit-1000.016 (due new precendence warning) #3

Open andk opened 3 weeks ago

andk commented 3 weeks ago

Fail report: http://www.cpantesters.org/cpan/report/ea122652-7dc1-11ef-b2e8-37781cd7e676

Bisect leads to

commit 570fa4332848e5e6d63ff907f85dd41234d6fe2c
Author: Lukas Mai <l.mai@web.de>
Date:   Tue Aug 13 15:49:43 2024 +0200

    warn about !$x == $y, !$x =~ /abc/, and similar constructs

    Commit 2f48e46b3f6d2d introduced a warning for logical negation as the
    left operand of the `isa` operator, which likely indicates a precedence
    problem (i.e. the programmer wrote `! $x isa $y` when they probably
    meant `!($x isa $y)`).

    This commit extends the warning to all (in)equality comparisons (`==`,
    `!=`, `<`, `>`, `<=`, `>=`, `eq`, `ne`, `lt`, `gt`, `le`, `ge`) as well
    as binding operations (`=~`, `!~`).

    As an indication that such a warning is useful in the real world, the
    core currently contains two files with (likely broken) code that
    triggers this warning:

      - ./cpan/Test-Simple/lib/Test2/Hub.pm
      - ./cpan/Scalar-List-Utils/t/uniqnum.t
jimav commented 3 weeks ago

This change creates trouble for the idom

if (!!$foo eq !!$bar) { ... }

used to compare values as boolean without caring about the specific form. In other words 0, undef, and "" are all equally false, and all non-zero numbers and non-empty strings are equally true.

I suggest making an explict !! exempt from this warning because it seems (to me) extremely unlikely that a programmer would expect ! to have lower precedence than 'eq' etc. when used in this way.

mauke commented 3 weeks ago

!ANY eq !ANY is already exempt from this warning. Note that the warning in your code says "… between ! and string ne", not "… string eq". The problem is !!$nopts{$key} ne $new, which does not have a ! on the right-hand side.

I suggest changing this expression to !!$nopts{$key} ne !!$new or (!!$nopts{$key}) ne $new or $new ne !!$nopts{$key}, either of which will avoid the warning.

jimav commented 3 weeks ago

Ok, thanks for pointing that out. I released an update.