search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.98k stars 559 forks source link

[PATCH] Move variables to their innermost scope that they're actually used #15874

Closed p5pRT closed 7 years ago

p5pRT commented 7 years ago

Migrated from rt.perl.org#130799 (status was 'rejected')

Searchable as RT130799$

p5pRT commented 7 years ago

From @petdance

Created by @petdance

This patch moves variables to the innermost scope where they're actually used. This will minimize the chances of variables being misused incorrectly\, and make clear the intended range of use of them.

In some cases\, temporary variables in inner scopes have been duplicated into two or more inner scopes.

In two cases\, this also saves us from initializing a variable when it doesn't need to be.

Perl Info ``` Flags: category=core severity=low Type=Patch PatchStatus=HasPatch Site configuration information for perl 5.24.0: Configured by andy at Sun Jun 5 23:28:46 CDT 2016. Summary of my perl5 (revision 5 version 24 subversion 0) configuration: Platform: osname=linux, osvers=3.10.0-327.18.2.el7.x86_64, archname=x86_64-linux uname='linux clifford 3.10.0-327.18.2.el7.x86_64 #1 smp thu may 12 11:03:55 utc 2016 x86_64 x86_64 x86_64 gnulinux ' config_args='-de -Dprefix=/home/andy/perl5/perlbrew/perls/perl-5.24.0 -Aeval:scriptdir=/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin' hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef use64bitint=define, use64bitall=define, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2', optimize='-O2', cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='', gccversion='4.8.5 20150623 (Red Hat 4.8.5-4)', gccosandvers='' intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3 ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='cc', ldflags =' -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib /lib64 /usr/lib64 /usr/local/lib64 libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.17.so, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.17' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong' Locally applied patches: Devel::PatchPerl 1.38 @INC for perl 5.24.0: /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0/x86_64-linux /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0 /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0/x86_64-linux /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0 . Environment for perl 5.24.0: HOME=/home/andy LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin:/home/andy/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin PERLBREW_BASHRC_VERSION=0.75 PERLBREW_HOME=/home/andy/.perlbrew PERLBREW_MANPATH=/home/andy/perl5/perlbrew/perls/perl-5.24.0/man PERLBREW_PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin PERLBREW_PERL=perl-5.24.0 PERLBREW_ROOT=/home/andy/perl5/perlbrew PERLBREW_VERSION=0.75 PERLCRITIC=/home/andy/tw/Dev/perlcriticrc PERL_BADLANG (unset) SHELL=/bin/bash ```
p5pRT commented 7 years ago

From @petdance

0001-Move-variables-to-the-scope-in-which-they-are-actual.patch ```diff From 63c794a20e8c736025e2840ea288f4f1c838eb58 Mon Sep 17 00:00:00 2001 From: Andy Lester Date: Thu, 16 Feb 2017 23:41:27 -0600 Subject: [PATCH] Move variables to the scope in which they are actually used. --- doop.c | 10 +++++++--- mg.c | 7 +++---- op.c | 7 ++++--- perl.c | 3 ++- pp.c | 11 +++++------ pp_hot.c | 5 ++++- pp_sys.c | 9 +++------ taint.c | 3 ++- toke.c | 12 ++++++------ util.c | 8 +++----- 10 files changed, 39 insertions(+), 36 deletions(-) diff --git a/doop.c b/doop.c index e4b2cd8..be70032 100644 --- a/doop.c +++ b/doop.c @@ -1065,16 +1065,16 @@ Perl_do_vop(pTHX_ I32 optype, SV *sv, SV *left, SV *right) dc = SvPVX(sv); /* sv_usepvn() calls Renew() */ } if (left_utf || right_utf) { - UV duc, luc, ruc; - char *dcorig = dc; + char * const dcorig = dc; char *dcsave = NULL; STRLEN lulen = leftlen; STRLEN rulen = rightlen; - STRLEN ulen; switch (optype) { case OP_BIT_AND: while (lulen && rulen) { + UV duc, luc, ruc; + STRLEN ulen; luc = utf8n_to_uvchr((U8*)lc, lulen, &ulen, UTF8_ALLOW_ANYUV); lc += ulen; lulen -= ulen; @@ -1097,6 +1097,8 @@ Perl_do_vop(pTHX_ I32 optype, SV *sv, SV *left, SV *right) break; case OP_BIT_XOR: while (lulen && rulen) { + UV duc, luc, ruc; + STRLEN ulen; luc = utf8n_to_uvchr((U8*)lc, lulen, &ulen, UTF8_ALLOW_ANYUV); lc += ulen; lulen -= ulen; @@ -1114,6 +1116,8 @@ Perl_do_vop(pTHX_ I32 optype, SV *sv, SV *left, SV *right) goto mop_up_utf; case OP_BIT_OR: while (lulen && rulen) { + UV duc, luc, ruc; + STRLEN ulen; luc = utf8n_to_uvchr((U8*)lc, lulen, &ulen, UTF8_ALLOW_ANYUV); lc += ulen; lulen -= ulen; diff --git a/mg.c b/mg.c index 6e648d8..b11f66a 100644 --- a/mg.c +++ b/mg.c @@ -607,9 +607,9 @@ Perl_mg_free_type(pTHX_ SV *sv, int how) MAGIC *mg, *prevmg, *moremg; PERL_ARGS_ASSERT_MG_FREE_TYPE; for (prevmg = NULL, mg = SvMAGIC(sv); mg; prevmg = mg, mg = moremg) { - MAGIC *newhead; moremg = mg->mg_moremagic; if (mg->mg_type == how) { + MAGIC *newhead; /* temporarily move to the head of the magic chain, in case custom free code relies on this historical aspect of mg_free */ if (prevmg) { @@ -1138,9 +1138,9 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg) #ifdef HAS_GETGROUPS { Groups_t *gary = NULL; - I32 i; I32 num_groups = getgroups(0, gary); if (num_groups > 0) { + I32 i; Newx(gary, num_groups, Groups_t); num_groups = getgroups(num_groups, gary); for (i = 0; i < num_groups; i++) @@ -2149,7 +2149,6 @@ Perl_magic_setpos(pTHX_ SV *sv, MAGIC *mg) SV* const lsv = LvTARG(sv); SSize_t pos; STRLEN len; - STRLEN ulen = 0; MAGIC* found; const char *s; @@ -2171,7 +2170,7 @@ Perl_magic_setpos(pTHX_ SV *sv, MAGIC *mg) pos = SvIV(sv); if (DO_UTF8(lsv)) { - ulen = sv_or_pv_len_utf8(lsv, s, len); + const STRLEN ulen = sv_or_pv_len_utf8(lsv, s, len); if (ulen) len = ulen; } diff --git a/op.c b/op.c index f16c6b5..21f7837 100644 --- a/op.c +++ b/op.c @@ -487,13 +487,13 @@ void Perl_opslab_force_free(pTHX_ OPSLAB *slab) { OPSLAB *slab2; - OPSLOT *slot; #ifdef DEBUGGING size_t savestack_count = 0; #endif PERL_ARGS_ASSERT_OPSLAB_FORCE_FREE; slab2 = slab; do { + OPSLOT *slot; for (slot = slab2->opslab_first; slot->opslot_next; slot = slot->opslot_next) { @@ -1865,7 +1865,6 @@ Perl_scalarvoid(pTHX_ OP *arg) dVAR; OP *kid; SV* sv; - U8 want; SSize_t defer_stack_alloc = 0; SSize_t defer_ix = -1; OP **defer_stack = NULL; @@ -1874,6 +1873,7 @@ Perl_scalarvoid(pTHX_ OP *arg) PERL_ARGS_ASSERT_SCALARVOID; do { + U8 want; SV *useless_sv = NULL; const char* useless = NULL; @@ -10895,7 +10895,6 @@ Perl_ck_require(pTHX_ OP *o) if (o->op_flags & OPf_KIDS) { /* Shall we supply missing .pm? */ SVOP * const kid = (SVOP*)cUNOPo->op_first; - HEK *hek; U32 hash; char *s; STRLEN len; @@ -10905,6 +10904,7 @@ Perl_ck_require(pTHX_ OP *o) if (kid->op_private & OPpCONST_BARE) { dVAR; const char *end; + HEK *hek; if (was_readonly) { SvREADONLY_off(sv); @@ -10947,6 +10947,7 @@ Perl_ck_require(pTHX_ OP *o) } else { dVAR; + HEK *hek; if (was_readonly) SvREADONLY_off(sv); PERL_HASH(hash, s, len); hek = share_hek(s, diff --git a/perl.c b/perl.c index 98bf356..658f260 100644 --- a/perl.c +++ b/perl.c @@ -1580,7 +1580,6 @@ perl_parse(pTHXx_ XSINIT_t xsinit, int argc, char **argv, char **env) * the original argv[0]. (See below for 'contiguous', though.) * --jhi */ const char *s = NULL; - int i; const UV mask = ~(UV)(PTRSIZE-1); /* Do the mask check only if the args seem like aligned. */ const UV aligned = @@ -1596,6 +1595,7 @@ perl_parse(pTHXx_ XSINIT_t xsinit, int argc, char **argv, char **env) * like the argv[] interleaved with some other data, we are * fine. (Did I just evoke Murphy's Law?) --jhi */ if (PL_origargv && PL_origargc >= 1 && (s = PL_origargv[0])) { + int i; while (*s) s++; for (i = 1; i < PL_origargc; i++) { if ((PL_origargv[i] == s + 1 @@ -1629,6 +1629,7 @@ perl_parse(pTHXx_ XSINIT_t xsinit, int argc, char **argv, char **env) INT2PTR(char *, PTR2UV(s + PTRSIZE) & mask))) ) { + int i; #ifndef OS2 /* ENVIRON is read by the kernel too. */ s = PL_origenviron[0]; while (*s) s++; diff --git a/pp.c b/pp.c index 3e6b891..4bd5f0f 100644 --- a/pp.c +++ b/pp.c @@ -5652,8 +5652,6 @@ PP(pp_reverse) } else { char *up; - char *down; - I32 tmp; dTARGET; STRLEN len; @@ -5666,6 +5664,7 @@ PP(pp_reverse) up = SvPV_force(TARG, len); if (len > 1) { + char *down; if (DO_UTF8(TARG)) { /* first reverse each character */ U8* s = (U8*)SvPVX(TARG); const U8* send = (U8*)(s + len); @@ -5682,9 +5681,9 @@ PP(pp_reverse) down = (char*)(s - 1); /* reverse this character */ while (down > up) { - tmp = *up; + const char tmp = *up; *up++ = *down; - *down-- = (char)tmp; + *down-- = tmp; } } } @@ -5692,9 +5691,9 @@ PP(pp_reverse) } down = SvPVX(TARG) + len - 1; while (down > up) { - tmp = *up; + const char tmp = *up; *up++ = *down; - *down-- = (char)tmp; + *down-- = tmp; } (void)SvPOK_only_UTF8(TARG); } diff --git a/pp_hot.c b/pp_hot.c index 7d6db0f..58bbe2f 100644 --- a/pp_hot.c +++ b/pp_hot.c @@ -360,7 +360,6 @@ PP(pp_padrange) dSP; PADOFFSET base = PL_op->op_targ; int count = (int)(PL_op->op_private) & OPpPADRANGE_COUNTMASK; - int i; if (PL_op->op_flags & OPf_SPECIAL) { /* fake the RHS of my ($x,$y,..) = @_ */ PUSHMARK(SP); @@ -370,6 +369,8 @@ PP(pp_padrange) /* note, this is only skipped for compile-time-known void cxt */ if ((PL_op->op_flags & OPf_WANT) != OPf_WANT_VOID) { + int i; + EXTEND(SP, count); PUSHMARK(SP); for (i = 0; i > (OPpPADRANGE_COUNTSHIFT+SAVE_TIGHT_SHIFT)) == (Size_t)base); diff --git a/pp_sys.c b/pp_sys.c index 9874107..7a57035 100644 --- a/pp_sys.c +++ b/pp_sys.c @@ -1432,7 +1432,6 @@ PP(pp_enterwrite) IO *io; GV *fgv; CV *cv = NULL; - SV *tmpsv = NULL; if (MAXARG == 0) { EXTEND(SP, 1); @@ -1456,7 +1455,7 @@ PP(pp_enterwrite) cv = GvFORM(fgv); if (!cv) { - tmpsv = sv_newmortal(); + SV * const tmpsv = sv_newmortal(); gv_efullname4(tmpsv, fgv, NULL, FALSE); DIE(aTHX_ "Undefined format \"%" SVf "\" called", SVfARG(tmpsv)); } @@ -4439,10 +4438,9 @@ PP(pp_system) if (did_pipes) { int errkid; unsigned n = 0; - SSize_t n1; while (n < sizeof(int)) { - n1 = PerlLIO_read(pp[0], + const SSize_t n1 = PerlLIO_read(pp[0], (void*)(((char*)&errkid)+n), (sizeof(int)) - n); if (n1 <= 0) @@ -4851,7 +4849,6 @@ PP(pp_alarm) PP(pp_sleep) { dSP; dTARGET; - I32 duration; Time_t lasttime; Time_t when; @@ -4859,7 +4856,7 @@ PP(pp_sleep) if (MAXARG < 1 || (!TOPs && !POPs)) PerlProc_pause(); else { - duration = POPi; + const I32 duration = POPi; if (duration < 0) { /* diag_listed_as: %s() with negative argument */ Perl_ck_warner_d(aTHX_ packWARN(WARN_MISC), diff --git a/taint.c b/taint.c index f1f6b7b..1b78928 100644 --- a/taint.c +++ b/taint.c @@ -78,7 +78,6 @@ void Perl_taint_env(pTHX) { SV** svp; - MAGIC* mg; const char* const *e; static const char* const misc_env[] = { "IFS", /* most shells' inter-field separators */ @@ -121,6 +120,7 @@ Perl_taint_env(pTHX) STRLEN len = 8; /* strlen(name) */ while (1) { + MAGIC* mg; if (i) len = my_sprintf(name,"DCL$PATH;%d", i); svp = hv_fetch(GvHVn(PL_envgv), name, len, FALSE); @@ -141,6 +141,7 @@ Perl_taint_env(pTHX) svp = hv_fetchs(GvHVn(PL_envgv),"PATH",FALSE); if (svp && *svp) { + MAGIC* mg; if (SvTAINTED(*svp)) { TAINT; taint_proper("Insecure %s%s", "$ENV{PATH}"); diff --git a/toke.c b/toke.c index 6d0ab62..2d4788c 100644 --- a/toke.c +++ b/toke.c @@ -3275,18 +3275,18 @@ S_scan_const(pTHX_ char *start) #endif /* Always gets run for ASCII, and sometimes for EBCDIC. */ { - SSize_t i; - /* Here, no conversions are necessary, which means that the * first character in the range is already in 'd' and * valid, so we can skip overwriting it */ if (has_utf8) { + IV i; d += UTF8SKIP(d); for (i = range_min + 1; i <= range_max; i++) { append_utf8_from_native_byte((U8) i, (U8 **) &d); } } else { + IV i; d++; assert(range_min + 1 <= range_max); for (i = range_min + 1; i < range_max; i++) { @@ -9040,7 +9040,6 @@ S_checkcomma(pTHX_ const char *s, const char *name, const char *what) s++; if (*s == ',') { GV* gv; - PADOFFSET off; if (keyword(w, s - w, 0)) return; @@ -9048,6 +9047,7 @@ S_checkcomma(pTHX_ const char *s, const char *name, const char *what) if (gv && GvCVu(gv)) return; if (s - w <= 254) { + PADOFFSET off; char tmpbuf[256]; Copy(w, tmpbuf+1, s - w, char); *tmpbuf = '&'; @@ -11335,8 +11335,6 @@ Perl_scan_num(pTHX_ const char *start, YYSTYPE* lvalp) STATIC char * S_scan_formline(pTHX_ char *s) { - char *eol; - char *t; SV * const stuff = newSVpvs(""); bool needargs = FALSE; bool eofmt = FALSE; @@ -11344,8 +11342,9 @@ S_scan_formline(pTHX_ char *s) PERL_ARGS_ASSERT_SCAN_FORMLINE; while (!needargs) { + char *eol; if (*s == '.') { - t = s+1; + char *t = s+1; #ifdef PERL_STRICT_CR while (SPACE_OR_TAB(*t)) t++; @@ -11362,6 +11361,7 @@ S_scan_formline(pTHX_ char *s) if (!eol++) eol = PL_bufend; if (*s != '#') { + char *t; for (t = s; t < eol; t++) { if (*t == '~' && t[1] == '~' && SvCUR(stuff)) { needargs = FALSE; diff --git a/util.c b/util.c index f119f3c..406286c 100644 --- a/util.c +++ b/util.c @@ -619,11 +619,11 @@ Perl_ninstr(const char *big, const char *bigend, const char *little, const char return (char*)big; { const char first = *little; - const char *s, *x; bigend -= lend - little++; OUTER: while (big <= bigend) { if (*big++ == first) { + const char *s, *x; for (x=big,s=little; s < lend; x++,s++) { if (*s != *x) goto OUTER; @@ -2540,10 +2540,9 @@ Perl_my_popen_list(pTHX_ const char *mode, int n, SV **args) if (did_pipes && pid > 0) { int errkid; unsigned n = 0; - SSize_t n1; while (n < sizeof(int)) { - n1 = PerlLIO_read(pp[0], + const SSize_t n1 = PerlLIO_read(pp[0], (void*)(((char*)&errkid)+n), (sizeof(int)) - n); if (n1 <= 0) @@ -2698,10 +2697,9 @@ Perl_my_popen(pTHX_ const char *cmd, const char *mode) if (did_pipes && pid > 0) { int errkid; unsigned n = 0; - SSize_t n1; while (n < sizeof(int)) { - n1 = PerlLIO_read(pp[0], + const SSize_t n1 = PerlLIO_read(pp[0], (void*)(((char*)&errkid)+n), (sizeof(int)) - n); if (n1 <= 0) -- 1.8.3.1 ```
p5pRT commented 7 years ago

From @hvds

On Thu\, 16 Feb 2017 22​:37​:04 -0800\, petdance wrote​:

This patch moves variables to the innermost scope where they're actually used. This will minimize the chances of variables being misused incorrectly\, and make clear the intended range of use of them.

Please could you make the patch _only_ that\, without the apparently random extra changes? I see const being added on variables not being moved\, and a change of a variable's type.

That'd make it easier to review\, and help ensure the commit message can accurately describe the change.

In particular I'd expect to see a separate commit for the SSize_t => IV change\, that explains why it's correct and necessary.

Hugo

p5pRT commented 7 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 7 years ago

From @petdance

In particular I'd expect to see a separate commit for the SSize_t => IV change\, that explains why it's correct and necessary.

Sorry\, I thought I’d removed that. I’ll rebuild the patch.

-- Andy Lester => www.petdance.com

p5pRT commented 7 years ago

From @petdance

Redone as #130809

p5pRT commented 7 years ago

@petdance - Status changed from 'open' to 'rejected'