develooper Front page | perl.perl5.porters | Postings from December 2018

Re: regexec.c: LGTM analysis warns about two comparisons, but weactually need them

Thread Previous | Thread Next
From:
Tony Cook
Date:
December 6, 2018 08:16
Subject:
Re: regexec.c: LGTM analysis warns about two comparisons, but weactually need them
Message ID:
20181206081641.5kndefoktkfeiczm@mars.tony.develop-help.com
On Wed, Dec 05, 2018 at 10:28:54PM -0500, James E Keenan wrote:
> Here is a case where the LGTM.com analysis of the Perl 5 source code
> produces results that are at first plausible but ultimately incorrect.
> 
> The following section of regexec.c is cited (https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804)
> with the warning-level alert "Comparison result is always the same":

This was discussed in #133686, the comparison needs to stay to allow
for local configuration of larger values of NUM_ANYOF_CODE_POINTS.

> I then created a branch (jkeenan/lgtm-regexec-c; https://perl5.git.perl.org/perl.git/commitdiff/1a6c6e69bfa70bc55eb0d1e85538d7420659a652)
> in which I simply removed those two comparisons.  Notwithstanding the
> alleged superfluousness of the two comparisons, I got these failures:
> 
> #####
> Test Summary Report
> -------------------
> re/regexp_noamp.t                                                (Wstat: 0
> Tests: 2006 Failed: 4)
>   Failed tests:  1830-1831, 1838-1839
> re/regexp.t                                                      (Wstat: 0
> Tests: 2006 Failed: 4)
>   Failed tests:  1830-1831, 1838-1839

The tests are failing because of the added
_CHECK_AND_OUTPUT_WIDE_LOCALE_CP_MSG(c);, from your commit:

diff --git a/regexec.c b/regexec.c
index eb24567e04..a265cc84cb 100644
--- a/regexec.c
+++ b/regexec.c
@@ -9772,15 +9772,13 @@ S_reginclass(pTHX_ regexp * const prog, const regnode * const n, const U8* const
            match = TRUE;
        }
        else if (flags & ANYOF_LOCALE_FLAGS) {
+            _CHECK_AND_OUTPUT_WIDE_LOCALE_CP_MSG(c);
            if ((flags & ANYOFL_FOLD)
-                && c < 256
                && ANYOF_BITMAP_TEST(n, PL_fold_locale[c]))
             {
                 match = TRUE;


Remove that and they pass.

> So we cannot remove the two comparisons and must somehow inform LGTM that
> these warnings are spurious.
> 
> Can anyone explain why these unit tests should preclude removal of the two
> comparisons?

As was discussed with you in #p5p and mentioned in #133686 these
comparisons are needed to alloww for local configuration of
NUM_ANYOF_CODE_POINTS.

Since NUM_ANYOF_CODE_POINTS is 256 (1<<8) by default LGTM considers
the comparisons superfluous, but if NUM_ANYOF_CODE_POINTS is defined
to 512 (or perhaps larger) to cover the Latin characters) the
comparisons against 256 later in the code would no longer be
superfluous.

The documented LGTM mechanism to suppress a warning is a "// lgtm"
comment[1], but since we support C89 we cannot use such comments.

Since LGTM doesn't provide an admin UI like coverity does for marking
warnings as "working as intended", we just have to live with LGTM
warning about that code.

Tony

[1] https://lgtm.com/help/lgtm/alert-suppression#c-c++

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About