Skip Menu |
Report information
Id: 133686
Status: resolved
Priority: 0/
Queue: perl5

Owner: jkeenan <jkeenan [at] cpan.org>
Requestors: jkeenan [at] pobox.com
Cc:
AdminCc:

Operating System: All
PatchStatus: HasPatch
Severity: low
Type: core
Perl Version: (no value)
Fixed In: (no value)

Attachments
0001-Eliminate-empty-conditional-branch.patch
0001-perl-133686-add-tests-for-the-Perl_cast_-functions.patch
0001-Remove-2-numerical-comparisons-which-are-always-true.patch
0001-Rename-global-variable-to-prevent-confusion-with-loc.patch
0001-Rename-local-variables-to-prevent-confusion-with-glo.patch
0002-Remove-2-numerical-comparisons-which-are-always-true.patch
0002-Rename-local-variable-to-prevent-confusion-with-glob.patch
0003-Remove-1-comparison-whose-result-is-always-the-same.patch
perl_V.3db0bccc.txt



To: perlbug [...] perl.org
Date: Sun, 25 Nov 2018 16:25:41 -0500
Subject: LGTM-derived analysis of alerts and patches
From: James E Keenan <jkeenan [...] pobox.com>
Last week Dominic Hargreaves provided patches which enable Perl 5 source code to be analyzed by LGTM.com. Dominic described LGTM as "... a code analysis tool which offers a powerful framework for software analysis, with a growing focus on security analysis and customisable queries." Over the past several days I looked at the LGTM-analysis of our codebase at https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree, which, as of this moment is reporting "141 alerts: 3 Errors, 104 Warnings, 34 Recommendations." I decided to see if any of these alerts were such low-hanging fruit that even I could pluck them. In this RT I comment on 4 of those alert categories and provide patches for 3 of them. I. Recommendation: Empty branch of conditional lgtm url: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803 branch: smoke-me/jkeenan/empty-conditional-branch branch url: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/empty-conditional-branch patch: 0001-Eliminate-empty-conditional-branch.patch # sv.c No smoke-test failures attributable to this patch (attached). Please review. II. Warning: local variable hides global variable lgtm url: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 branch: smoke-me/jkeenan/local-hides-global branch url: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/local-hides-global patch: 0001-Rename-local-variables-to-prevent-confusion-with-glo.patch # locale.c universal.c No smoke-test failures attributable to this patch (attached). Please review. III. Warning: Comparison result is always the same lgtm url: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 branch: smoke-me/jkeenan/compare-result-always-same branch url: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch # regexec.c 0002-Remove-2-numerical-comparisons-which-are-always-true.patch # numeric.c # TonyC says no 0003-Remove-1-comparison-whose-result-is-always-the-same.patch # pp_sys.c No smoke-test failures attributable to these patches (attached). However, on #p5p Tony Cook advised against applying the 0002 patch for numeric.c: "If the numeric.c changes don't result in a failure, maybe we're missing a test or two. ... [T]hose numeric.c functions don't seem to be used in core, but they're part of the API. [S]o they could be tested in XS-APItest." Please review the other two. IV. Error: Assignment where comparison was intended lgtm url: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641 branch: jkeenan/assignment-vs-comparison branch url: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparison Though lgtm characterizes this as an error, "fixing" it causes extensive test failures in t/re/fold_grind.t. Hence, I did not submit the branch for smoking and am not attaching any patch. Thank you very much. Jim Keenan

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

To: Perl5 Porters <perl5-porters [...] perl.org>
Subject: Re: [perl #133686] LGTM-derived analysis of alerts and patches
Date: Sun, 25 Nov 2018 22:52:11 +0100
From: Leon Timmermans <fawaka [...] gmail.com>
CC: bugs-bitbucket [...] rt.perl.org
Download (untitled) / with headers
text/plain 1.1k
On Sun, Nov 25, 2018 at 10:26 PM James E Keenan (via RT) <perlbug-followup@perl.org> wrote: Show quoted text
> III. > > Warning: Comparison result is always the same > lgtm url: > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 > branch: smoke-me/jkeenan/compare-result-always-same > branch url: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same > patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch > # regexec.c
I suspect Karl needs to look at that before we apply. If that is a left-over that really should have been NUM_ANYOF_CODE_POINTS then indeed it can go. Show quoted text
> IV. > > Error: Assignment where comparison was intended > lgtm url: > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641 > branch: jkeenan/assignment-vs-comparison > branch url: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparison > > Though lgtm characterizes this as an error, "fixing" it causes extensive > test failures in t/re/fold_grind.t. Hence, I did not submit the branch > for smoking and am not attaching any patch.
Yeah that looks quite intentional to me. Leon
From: Tony Cook <tony [...] develop-help.com>
Subject: Re: [perl #133686] LGTM-derived analysis of alerts and patches
Date: Mon, 26 Nov 2018 11:54:59 +1100
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 3.9k
Show quoted text
> Last week Dominic Hargreaves provided patches which enable Perl 5 source > code to be analyzed by LGTM.com. Dominic described LGTM as "... a code > analysis tool which offers a powerful framework for software analysis, > with a growing focus on security analysis and customisable queries." > > Over the past several days I looked at the LGTM-analysis of our codebase > at https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree, which, as > of this moment is reporting "141 alerts: 3 Errors, 104 Warnings, 34 > Recommendations." I decided to see if any of these alerts were such > low-hanging fruit that even I could pluck them. In this RT I comment on > 4 of those alert categories and provide patches for 3 of them. > > I. > > Recommendation: Empty branch of conditional > lgtm url: > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803 > branch: smoke-me/jkeenan/empty-conditional-branch > branch url: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/empty-conditional-branch > patch: 0001-Eliminate-empty-conditional-branch.patch # sv.c > > No smoke-test failures attributable to this patch (attached). Please > review.
This patch is reasonable I think. Show quoted text
> Warning: local variable hides global variable > lgtm url: > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 > branch: smoke-me/jkeenan/local-hides-global > branch url: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/local-hides-global > patch: 0001-Rename-local-variables-to-prevent-confusion-with-glo.patch # > locale.c universal.c > > No smoke-test failures attributable to this patch (attached). Please > review.
Renaming the global (which is actually a static) for universal.c makes more sense to me. Similarly for locale.c, though you might want to pass that by khw. Show quoted text
> > III. > > Warning: Comparison result is always the same > lgtm url: > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 > branch: smoke-me/jkeenan/compare-result-always-same > branch url: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same > patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch > # regexec.c
While this isn't exactly a false positive, as discussed in #p5p, it would break using a larger value for NUM_ANYOF_CODE_POINTS, so it is a false positive for our purposes. Leon suggested that the c < 256 comparison might be incorrect, but no, it's checking whether the character is foldable with the PL_fold_locale[] lookup table. So patching out this comparison would be incorrect. Show quoted text
> 0002-Remove-2-numerical-comparisons-which-are-always-true.patch # > numeric.c # TonyC says no
See the attached patch for tests that demonstrate why this is a false positive. Also: https://discuss.lgtm.com/t/false-positive-c-for-floating-point/1567 Show quoted text
> 0003-Remove-1-comparison-whose-result-is-always-the-same.patch > # pp_sys.c
This one (in pp_readlink) seems reasonable. Show quoted text
> No smoke-test failures attributable to these patches (attached). > However, on #p5p Tony Cook advised against applying the 0002 patch for > numeric.c: "If the numeric.c changes don't result in a failure, maybe > we're missing a test or two. ... [T]hose numeric.c functions don't seem > to be used in core, but they're part of the API. [S]o they could be > tested in XS-APItest." Please review the other two.
Show quoted text
> > IV. > > Error: Assignment where comparison was intended > lgtm url: > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641 > branch: jkeenan/assignment-vs-comparison > branch url: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparison > > Though lgtm characterizes this as an error, "fixing" it causes extensive > test failures in t/re/fold_grind.t. Hence, I did not submit the branch > for smoking and am not attaching any patch.
Coverity picked this up too, and we suppressed it (the code is correct.) Tony

Message body is not shown because sender requested not to inline it.

From: Karl Williamson <public [...] khwilliamson.com>
Date: Sun, 25 Nov 2018 18:30:17 -0700
Subject: Re: [perl #133686] LGTM-derived analysis of alerts and patches
To: Tony Cook <tony [...] develop-help.com>, perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 4.5k
On 11/25/18 5:54 PM, Tony Cook wrote: Show quoted text
>> Last week Dominic Hargreaves provided patches which enable Perl 5 source >> code to be analyzed by LGTM.com. Dominic described LGTM as "... a code >> analysis tool which offers a powerful framework for software analysis, >> with a growing focus on security analysis and customisable queries." >> >> Over the past several days I looked at the LGTM-analysis of our codebase >> at https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree, which, as >> of this moment is reporting "141 alerts: 3 Errors, 104 Warnings, 34 >> Recommendations." I decided to see if any of these alerts were such >> low-hanging fruit that even I could pluck them. In this RT I comment on >> 4 of those alert categories and provide patches for 3 of them. >> >> I. >> >> Recommendation: Empty branch of conditional >> lgtm url: >> https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803 >> branch: smoke-me/jkeenan/empty-conditional-branch >> branch url: >> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/empty-conditional-branch >> patch: 0001-Eliminate-empty-conditional-branch.patch # sv.c >> >> No smoke-test failures attributable to this patch (attached). Please >> review.
> > This patch is reasonable I think. >
>> Warning: local variable hides global variable >> lgtm url: >> https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 >> branch: smoke-me/jkeenan/local-hides-global >> branch url: >> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/local-hides-global >> patch: 0001-Rename-local-variables-to-prevent-confusion-with-glo.patch # >> locale.c universal.c >> >> No smoke-test failures attributable to this patch (attached). Please >> review.
> > Renaming the global (which is actually a static) for universal.c makes > more sense to me. > > Similarly for locale.c, though you might want to pass that by khw.
Without even reviewing the code, I think hiding a global is poor practice, so should be fixed. Show quoted text
>
>> >> III. >> >> Warning: Comparison result is always the same >> lgtm url: >> https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 >> branch: smoke-me/jkeenan/compare-result-always-same >> branch url: >> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/jkeenan/compare-result-always-same >> patches: 0001-Remove-2-numerical-comparisons-which-are-always-true.patch >> # regexec.c
> > While this isn't exactly a false positive, as discussed in #p5p, it > would break using a larger value for NUM_ANYOF_CODE_POINTS, so it is a > false positive for our purposes. > > Leon suggested that the c < 256 comparison might be incorrect, but no, > it's checking whether the character is foldable with the > PL_fold_locale[] lookup table.
And the other is checking that the character fits in the parameter size passed to a function. They could be #ifdef'd out, as is another instance of this in regexec.c (probably to silence a compiler warning), but I avoid #ifdef's when possible, as it makes the code harder to read. But it could be done for these if people think it's warranted. Show quoted text
> > So patching out this comparison would be incorrect. >
>> 0002-Remove-2-numerical-comparisons-which-are-always-true.patch # >> numeric.c # TonyC says no
> > See the attached patch for tests that demonstrate why this is a false > positive. > > Also: > > https://discuss.lgtm.com/t/false-positive-c-for-floating-point/1567 >
>> 0003-Remove-1-comparison-whose-result-is-always-the-same.patch >> # pp_sys.c
> > This one (in pp_readlink) seems reasonable. >
>> No smoke-test failures attributable to these patches (attached). >> However, on #p5p Tony Cook advised against applying the 0002 patch for >> numeric.c: "If the numeric.c changes don't result in a failure, maybe >> we're missing a test or two. ... [T]hose numeric.c functions don't seem >> to be used in core, but they're part of the API. [S]o they could be >> tested in XS-APItest." Please review the other two.
> > >
>> >> IV. >> >> Error: Assignment where comparison was intended >> lgtm url: >> https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2158670641 >> branch: jkeenan/assignment-vs-comparison >> branch url: >> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/jkeenan/assignment-vs-comparison >> >> Though lgtm characterizes this as an error, "fixing" it causes extensive >> test failures in t/re/fold_grind.t. Hence, I did not submit the branch >> for smoking and am not attaching any patch.
> > Coverity picked this up too, and we suppressed it (the code is correct.) > > Tony >
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.9k
On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote: Show quoted text
> > > > I. > > > > Recommendation: Empty branch of conditional > > lgtm url: > > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840803 > > branch: smoke-me/jkeenan/empty-conditional-branch > > branch url: > > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- > > me/jkeenan/empty-conditional-branch > > patch: 0001-Eliminate-empty-conditional-branch.patch # sv.c > > > > No smoke-test failures attributable to this patch (attached). Please > > review.
> > This patch is reasonable I think. >
Applied in commit 41b654eb4e6464cf37e5ee236d2800e73948778c. [snip] Show quoted text
>
> > > > III. > > > > Warning: Comparison result is always the same > > lgtm url: > > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804 > > branch: smoke-me/jkeenan/compare-result-always-same > > branch url: > > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- > > me/jkeenan/compare-result-always-same > > patches: 0001-Remove-2-numerical-comparisons-which-are-always- > > true.patch > > # regexec.c
> > While this isn't exactly a false positive, as discussed in #p5p, it > would break using a larger value for NUM_ANYOF_CODE_POINTS, so it is a > false positive for our purposes. > > Leon suggested that the c < 256 comparison might be incorrect, but no, > it's checking whether the character is foldable with the > PL_fold_locale[] lookup table. > > So patching out this comparison would be incorrect. >
> > 0002-Remove-2-numerical-comparisons-which-are-always-true.patch # > > numeric.c # TonyC says no
> > See the attached patch for tests that demonstrate why this is a false > positive. > > Also: > > https://discuss.lgtm.com/t/false-positive-c-for-floating-point/1567 >
> > 0003-Remove-1-comparison-whose-result-is-always-the-same.patch > > # pp_sys.c
> > This one (in pp_readlink) seems reasonable. >
Applied in commit 3a56a99d41d6fbc76d3de73c647f90cf10ea1992. Everything else still pends. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote: Show quoted text
>
> > Warning: local variable hides global variable > > lgtm url: > > https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 > > branch: smoke-me/jkeenan/local-hides-global > > branch url: > > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- > > me/jkeenan/local-hides-global > > patch: 0001-Rename-local-variables-to-prevent-confusion-with- > > glo.patch # > > locale.c universal.c > > > > No smoke-test failures attributable to this patch (attached). Please > > review.
> > Renaming the global (which is actually a static) for universal.c makes > more sense to me. > > Similarly for locale.c, though you might want to pass that by khw.
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 0002-Rename-local-variable-to-prevent-confusion-with-glob.patch Thank you very much. -- James E Keenan (jkeenan@cpan.org)
Subject: 0001-Rename-global-variable-to-prevent-confusion-with-loc.patch
From 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
Subject: 0002-Rename-local-variable-to-prevent-confusion-with-glob.patch
From 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
To: perlbug-followup [...] perl.org
Date: Mon, 26 Nov 2018 10:47:41 -0700
Subject: Re: [perl #133686] LGTM-derived analysis of alerts and patches
CC: perl5-porters [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 1.3k
On 11/26/18 10:39 AM, James E Keenan via RT wrote: Show quoted text
> On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
>>
>>> Warning: local variable hides global variable >>> lgtm url: >>> https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 >>> branch: smoke-me/jkeenan/local-hides-global >>> branch url: >>> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- >>> me/jkeenan/local-hides-global >>> patch: 0001-Rename-local-variables-to-prevent-confusion-with- >>> glo.patch # >>> locale.c universal.c >>> >>> No smoke-test failures attributable to this patch (attached). Please >>> review.
>> >> Renaming the global (which is actually a static) for universal.c makes >> more sense to me. >> >> Similarly for locale.c, though you might want to pass that by khw.
> > 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 > 0002-Rename-local-variable-to-prevent-confusion-with-glob.patch > > Thank you very much. >
Fine by me
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On Mon, 26 Nov 2018 17:48:21 GMT, public@khwilliamson.com wrote: Show quoted text
> On 11/26/18 10:39 AM, James E Keenan via RT wrote:
> > On Mon, 26 Nov 2018 00:55:29 GMT, tonyc wrote:
> >>
> >>> Warning: local variable hides global variable > >>> lgtm url: > >>> https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2157860312 > >>> branch: smoke-me/jkeenan/local-hides-global > >>> branch url: > >>> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- > >>> me/jkeenan/local-hides-global > >>> patch: 0001-Rename-local-variables-to-prevent-confusion-with- > >>> glo.patch # > >>> locale.c universal.c > >>> > >>> No smoke-test failures attributable to this patch (attached). > >>> Please > >>> review.
> >> > >> Renaming the global (which is actually a static) for universal.c > >> makes > >> more sense to me. > >> > >> Similarly for locale.c, though you might want to pass that by khw.
> > > > 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 > > 0002-Rename-local-variable-to-prevent-confusion-with-glob.patch > > > > Thank you very much. > >
> > Fine by me
Those two patches have been merged into blead. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 328b
I'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. -- James E Keenan (jkeenan@cpan.org)
Download (untitled) / with headers
text/plain 313b
Thank 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 resolved. Perl 5.30.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.30.0 If you find that the problem persists, feel free to reopen this ticket.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org