LGTM-derived analysis of alerts and patches #16765
Comments
From @jkeenanLast week Dominic Hargreaves provided patches which enable Perl 5 source Over the past several days I looked at the LGTM-analysis of our codebase I. Recommendation: Empty branch of conditional No smoke-test failures attributable to this patch (attached). Please II. Warning: local variable hides global variable No smoke-test failures attributable to this patch (attached). Please III. Warning: Comparison result is always the same 0002-Remove-2-numerical-comparisons-which-are-always-true.patch # No smoke-test failures attributable to these patches (attached). IV. Error: Assignment where comparison was intended Though lgtm characterizes this as an error, "fixing" it causes extensive Thank you very much. |
From @jkeenan0001-Rename-local-variables-to-prevent-confusion-with-glo.patchFrom ac5465a8c22263a206c773b7396c35710b520416 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 23 Nov 2018 12:51:44 -0500
Subject: [PATCH] Rename local variables to prevent confusion with globals.
Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
---
locale.c | 6 +++---
universal.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/locale.c b/locale.c
index ff6322fdc3..c32dd3f6ed 100644
--- a/locale.c
+++ b/locale.c
@@ -5050,15 +5050,15 @@ Perl__is_in_locale_category(pTHX_ const bool compiling, const int category)
const COP * const cop = (compiling) ? &PL_compiling : PL_curcop;
- SV *categories = cop_hints_fetch_pvs(cop, "locale", 0);
- if (! categories || categories == &PL_sv_placeholder) {
+ SV *these_categories = cop_hints_fetch_pvs(cop, "locale", 0);
+ if (! these_categories || these_categories == &PL_sv_placeholder) {
return FALSE;
}
/* The pseudo-category 'not_characters' is -1, so just add 1 to each to get
* a valid unsigned */
assert(category >= -1);
- return cBOOL(SvUV(categories) & (1U << (category + 1)));
+ return cBOOL(SvUV(these_categories) & (1U << (category + 1)));
}
char *
diff --git a/universal.c b/universal.c
index 2262939b8d..5a74722b30 100644
--- a/universal.c
+++ b/universal.c
@@ -655,7 +655,7 @@ XS(XS_PerlIO_get_layers)
GV * gv;
IO * io;
bool input = TRUE;
- bool details = FALSE;
+ bool these_details = FALSE;
if (items > 1) {
SV * const *svp;
@@ -680,7 +680,7 @@ XS(XS_PerlIO_get_layers)
goto fail;
case 'd':
if (memEQs(key, klen, "details")) {
- details = SvTRUE(*valp);
+ these_details = SvTRUE(*valp);
break;
}
goto fail;
@@ -718,7 +718,7 @@ XS(XS_PerlIO_get_layers)
const bool flgok = flgsvp && *flgsvp && SvIOK(*flgsvp);
EXTEND(SP, 3); /* Three is the max in all branches: better check just once */
- if (details) {
+ if (these_details) {
/* Indents of 5? Yuck. */
/* We know that PerlIO_get_layers creates a new SV for
the name and flags, so we can just take a reference
--
2.17.1
|
From @jkeenan0001-Eliminate-empty-conditional-branch.patchFrom 3e8060643f957359e16cb8e39c5b6cbee09e0cf6 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 23 Nov 2018 13:45:25 -0500
Subject: [PATCH] Eliminate empty conditional branch
Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803
---
sv.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sv.c b/sv.c
index b10f205033..826228bc92 100644
--- a/sv.c
+++ b/sv.c
@@ -5164,9 +5164,8 @@ S_sv_uncow(pTHX_ SV * const sv, const U32 flags)
SvCUR_set(sv, cur);
*SvEND(sv) = '\0';
}
- if (len) {
- } else {
- unshare_hek(SvSHARED_HEK_FROM_PV(pvx));
+ if (! len) {
+ unshare_hek(SvSHARED_HEK_FROM_PV(pvx));
}
#ifdef DEBUGGING
if (DEBUG_C_TEST)
--
2.17.1
|
From @jkeenan0001-Remove-2-numerical-comparisons-which-are-always-true.patchFrom e5151f105a0a15ec8086bbc68738c55c24bf052e Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 23 Nov 2018 10:37:37 -0500
Subject: [PATCH 1/3] Remove 2 numerical comparisons which are always true.
Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
---
regexec.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/regexec.c b/regexec.c
index be1f5aece3..52d9a64232 100644
--- a/regexec.c
+++ b/regexec.c
@@ -9753,13 +9753,11 @@ S_reginclass(pTHX_ regexp * const prog, const regnode * const n, const U8* const
}
else if (flags & ANYOF_LOCALE_FLAGS) {
if ((flags & ANYOFL_FOLD)
- && c < 256
&& ANYOF_BITMAP_TEST(n, PL_fold_locale[c]))
{
match = TRUE;
}
else if (ANYOF_POSIXL_TEST_ANY_SET(n)
- && c < 256
) {
/* The data structure is arranged so bits 0, 2, 4, ... are set
--
2.17.1
|
From @jkeenan0002-Remove-2-numerical-comparisons-which-are-always-true.patchFrom daaf264cc2c8a24a2b80a6482c48d6147946f86a Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 23 Nov 2018 10:51:21 -0500
Subject: [PATCH 2/3] Remove 2 numerical comparisons which are always true.
Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
---
numeric.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/numeric.c b/numeric.c
index e5e08cb241..f153f9924a 100644
--- a/numeric.c
+++ b/numeric.c
@@ -44,7 +44,7 @@ Perl_cast_ulong(NV f)
return (U32) f;
#endif
}
- return f > 0 ? U32_MAX : 0 /* NaN */;
+ return U32_MAX /* NaN */;
}
I32
@@ -62,7 +62,7 @@ Perl_cast_i32(NV f)
return (I32)(U32) f;
#endif
}
- return f > 0 ? (I32)U32_MAX : 0 /* NaN */;
+ return (I32)U32_MAX /* NaN */;
}
IV
--
2.17.1
|
From @jkeenan0003-Remove-1-comparison-whose-result-is-always-the-same.patchFrom f98cbe2b272b02a5937b8a1020894cfa088919eb Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 23 Nov 2018 11:49:03 -0500
Subject: [PATCH 3/3] Remove 1 comparison whose result is always the same.
Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
---
pp_sys.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/pp_sys.c b/pp_sys.c
index ab28ed65d9..5dc20b14f0 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -3824,8 +3824,7 @@ PP(pp_readlink)
len = readlink(tmps, buf, sizeof(buf) - 1);
if (len < 0)
RETPUSHUNDEF;
- if (len != -1)
- buf[len] = '\0';
+ buf[len] = '\0';
PUSHp(buf, len);
RETURN;
#else
--
2.17.1
|
From @jkeenanSummary of my perl5 (revision 5 version 29 subversion 5) configuration: Characteristics of this binary (from libperl): |
From @LeontOn Sun, Nov 25, 2018 at 10:26 PM James E Keenan (via RT)
I suspect Karl needs to look at that before we apply. If that is a
Yeah that looks quite intentional to me. Leon |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycoz
This patch is reasonable I think.
Renaming the global (which is actually a static) for universal.c makes Similarly for locale.c, though you might want to pass that by khw.
While this isn't exactly a false positive, as discussed in #p5p, it Leon suggested that the c < 256 comparison might be incorrect, but no, So patching out this comparison would be incorrect.
See the attached patch for tests that demonstrate why this is a false Also: https://discuss.lgtm.com/t/false-positive-c-for-floating-point/1567
This one (in pp_readlink) seems reasonable.
Coverity picked this up too, and we suppressed it (the code is correct.) Tony |
From @tonycoz0001-perl-133686-add-tests-for-the-Perl_cast_-functions.patchFrom 5e07ce045e5fb6e31d7cbedec9e19530d1f0363a Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 26 Nov 2018 11:15:58 +1100
Subject: (perl #133686) add tests for the Perl_cast_* functions
Prompted by a false positive by lgtm, which doesn't appear to know
about NaN.
---
MANIFEST | 1 +
ext/XS-APItest/numeric.xs | 12 ++++++++++
ext/XS-APItest/t/numeric.t | 59 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+)
create mode 100644 ext/XS-APItest/t/numeric.t
diff --git a/MANIFEST b/MANIFEST
index 3fdf274cf4..6c4ed65497 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -4321,6 +4321,7 @@ ext/XS-APItest/t/my_exit.t XS::APItest: test my_exit
ext/XS-APItest/t/newCONSTSUB.t XS::APItest: test newCONSTSUB(_flags)
ext/XS-APItest/t/newDEFSVOP.t XS::APItest: test newDEFSVOP
ext/XS-APItest/t/Null.pm Helper for ./blockhooks.t
+ext/XS-APItest/t/numeric.c XS::APItest: test numeric.c utility functions
ext/XS-APItest/t/op.t XS::APItest: tests for OP related APIs
ext/XS-APItest/t/op_contextualize.t test op_contextualize() API
ext/XS-APItest/t/op_list.t test OP list construction API
diff --git a/ext/XS-APItest/numeric.xs b/ext/XS-APItest/numeric.xs
index 847eb75d7c..22e9034661 100644
--- a/ext/XS-APItest/numeric.xs
+++ b/ext/XS-APItest/numeric.xs
@@ -59,3 +59,15 @@ grok_atoUV(number, endsv)
PUSHs(sv_2mortal(newSViv(0)));
}
}
+
+UV
+U_32(NV number)
+
+UV
+U_V(NV number)
+
+IV
+I_32(NV number)
+
+IV
+I_V(NV number)
diff --git a/ext/XS-APItest/t/numeric.t b/ext/XS-APItest/t/numeric.t
new file mode 100644
index 0000000000..71165d6ba9
--- /dev/null
+++ b/ext/XS-APItest/t/numeric.t
@@ -0,0 +1,59 @@
+#!perl
+use strict;
+use Test::More;
+
+use XS::APItest;
+use Config;
+
+unless ($Config{d_double_has_inf} && $Config{d_double_has_nan}) {
+ plan skip_all => "the doublekind $Config{doublekind} does not have inf/nan";
+}
+
+my $nan = "NaN" + 0;
+my $large = 256 ** ($Config{ivsize}+1);
+my $small = -$large;
+my $max = int(~0 / 2);
+my $min = -$max - 1;
+my $umax = ~0;
+my $i32_max = 0x7fffffff;
+my $i32_min = -$i32_max - 1;
+my $u32_max = 0xffffffff;
+my $i32_min_as_u32 = 0x80000000;
+my $iv_min_as_uv = 0x1 << ($Config{ivsize} * 8 - 1);
+
+*U_32 = \&XS::APItest::numeric::U_32;
+*I_32 = \&XS::APItest::numeric::I_32;
+*U_V = \&XS::APItest::numeric::U_V;
+*I_V = \&XS::APItest::numeric::I_V;
+
+is(U_32(0.0), 0, "zero to u32");
+is(U_32($nan), 0, "nan to u32");
+is(U_32($large), $u32_max, "large to u32");
+is(U_32($min), $i32_min_as_u32, "small to u32");
+is(U_32($u32_max), $u32_max, "u32_max to u32");
+
+is(I_32(0.0), 0, "zero to i32");
+is(I_32($nan), 0, "nan to i32");
+# this surprised me, but printf("%d"), which does iv = nv at the
+# end produces the same value
+is(I_32($large), -1, "large ($large) to i32");
+is(I_32($min), $i32_min, "small to i32");
+is(I_32($i32_max), $i32_max, "i32_max to i32");
+
+is(U_V(0.0), 0, "zero to uv");
+is(U_V($nan), 0, "nan to uv");
+is(U_V($large), $umax, "large to uv");
+is(U_V($min), $iv_min_as_uv, "small to uv");
+# a large UV might not be exactly representable in a double
+#is(U_V($umax), $umax, "u32_max to uv");
+
+is(I_V(0.0), 0, "zero to iv");
+is(I_V($nan), 0, "nan to iv");
+# this surprised me, but printf("%d"), which does iv = nv at the
+# end produces the same value
+is(I_V($large), -1, "large ($large) to iv");
+is(I_V($min), $iv_min, "small to iv");
+# a large IV might not be exactly representable in a double
+#is(I_V($iv_max), $iv_max, "iv_max to iv");
+
+done_testing();
--
2.11.0
|
From @khwilliamsonOn 11/25/18 5:54 PM, Tony Cook wrote:
Without even reviewing the code, I think hiding a global is poor
And the other is checking that the character fits in the parameter size
|
From @jkeenanOn Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
Applied in commit 41b654e. [snip]
Applied in commit 3a56a99. Everything else still pends. -- |
From @jkeenanOn Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
I agree with you in the case of universal.c. However, in the case of locale.c, there are many instances of the the global and the string 'categories' is frequently used in internal comments. So in this case confining the renaming to a small scope (the "local" instance) seems more prudent. See this new branch: smoke-me/jkeenan/133686-local-hides-global-2nd 2 new patches attached: 0001-Rename-global-variable-to-prevent-confusion-with-loc.patch Thank you very much. -- |
From @jkeenan0001-Rename-global-variable-to-prevent-confusion-with-loc.patchFrom 419f8ba5087ee6baa2281aa8c355fc66a0fe3851 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Mon, 26 Nov 2018 12:14:51 -0500
Subject: [PATCH 1/2] Rename global variable to prevent confusion with local
Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
---
universal.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/universal.c b/universal.c
index 2262939b8d..c1b5dd4b14 100644
--- a/universal.c
+++ b/universal.c
@@ -995,7 +995,7 @@ struct xsub_details {
const char *proto;
};
-static const struct xsub_details details[] = {
+static const struct xsub_details these_details[] = {
{"UNIVERSAL::isa", XS_UNIVERSAL_isa, NULL},
{"UNIVERSAL::can", XS_UNIVERSAL_can, NULL},
{"UNIVERSAL::DOES", XS_UNIVERSAL_DOES, NULL},
@@ -1075,8 +1075,8 @@ void
Perl_boot_core_UNIVERSAL(pTHX)
{
static const char file[] = __FILE__;
- const struct xsub_details *xsub = details;
- const struct xsub_details *end = C_ARRAY_END(details);
+ const struct xsub_details *xsub = these_details;
+ const struct xsub_details *end = C_ARRAY_END(these_details);
do {
newXS_flags(xsub->name, xsub->xsub, file, xsub->proto, 0);
--
2.17.1
|
From @jkeenan0002-Rename-local-variable-to-prevent-confusion-with-glob.patchFrom 363d7d4a2b1a2867eccb5132540f9055aca201ef Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Mon, 26 Nov 2018 12:29:03 -0500
Subject: [PATCH 2/2] Rename local variable to prevent confusion with global
Per: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312
For: RT # 133686 (partial)
---
locale.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/locale.c b/locale.c
index ff6322fdc3..c32dd3f6ed 100644
--- a/locale.c
+++ b/locale.c
@@ -5050,15 +5050,15 @@ Perl__is_in_locale_category(pTHX_ const bool compiling, const int category)
const COP * const cop = (compiling) ? &PL_compiling : PL_curcop;
- SV *categories = cop_hints_fetch_pvs(cop, "locale", 0);
- if (! categories || categories == &PL_sv_placeholder) {
+ SV *these_categories = cop_hints_fetch_pvs(cop, "locale", 0);
+ if (! these_categories || these_categories == &PL_sv_placeholder) {
return FALSE;
}
/* The pseudo-category 'not_characters' is -1, so just add 1 to each to get
* a valid unsigned */
assert(category >= -1);
- return cBOOL(SvUV(categories) & (1U << (category + 1)));
+ return cBOOL(SvUV(these_categories) & (1U << (category + 1)));
}
char *
--
2.17.1
|
From @khwilliamsonOn 11/26/18 10:39 AM, James E Keenan via RT wrote:
Fine by me |
From @jkeenanOn Mon, 26 Nov 2018 17:48:21 GMT, public@khwilliamson.com wrote:
Those two patches have been merged into blead. -- |
From @jkeenanI'm going to resolve this ticket because it appears that all the patches that can usefully be applied to blead have been applied. Also, going forward, for clarity in discussion we should probably open one RT for each combination of LGTM-warning and source-code file. Thank you very much. -- |
|
@jkeenan - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.30.0, this and 160 other issues have been Perl 5.30.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#133686 (status was 'resolved')
Searchable as RT133686$
The text was updated successfully, but these errors were encountered: