develooper Front page | perl.perl5.porters | Postings from July 2012

Re: [PATCH] v5: fix blead usage of OK flags vs. all magic

Thread Previous | Thread Next
From:
Father Chrysostomos
Date:
July 1, 2012 13:35
Subject:
Re: [PATCH] v5: fix blead usage of OK flags vs. all magic
Message ID:
8C68B508-AF8B-4144-9F8A-07F1864C5CD4@cpan.org

On Jun 28, 2012, at 12:54 PM, Rev. Chip wrote:

> On Sun, Jun 17, 2012 at 02:08:16PM -0700, Father Chrysostomos wrote:
>> diff --git a/cpan/Compress-Raw-Bzip2/Bzip2.xs b/cpan/Compress-Raw-Bzip2/Bzip2.xs
>> diff --git a/cpan/Compress-Raw-Zlib/Zlib.xs b/cpan/Compress-Raw-Zlib/Zlib.xs
>> 
>> Here you are touching files in cpan/, which is verboten.  Have you
>> submitted the change upstream?  Have you tested it in 5.006?
> 
> Yes, I did; and no, I didn't, but I shall.  But "verboten" is not the right
> way to think of this.

Well, it’s a German word, and I don’t speak German. :-)


> 
> ----------------------------------------------------------------
> 
>> diff --git a/doio.c b/doio.c
>> index fed1a49..64e6a73 100644
>> --- a/doio.c
>> +++ b/doio.c
>> @@ -747,6 +747,7 @@ Perl_nextargv(pTHX_ register GV *gv)
>> 	STRLEN oldlen;
>> 	sv = av_shift(GvAV(gv));
>> 	SAVEFREESV(sv);
>> +	SvTAINTED_off(GvSVn(gv)); /* previous tainting irrelevant */
>> 	sv_setsv(GvSVn(gv),sv);
>> 	SvSETMAGIC(GvSV(gv));
>> 	PL_oldname = SvPVx(GvSV(gv), oldlen);
>> 
>> [...] GvSVn twice in a row is a waste of CPU.
> 
> Well, yeah, a bit, but SvTAINTED_off() may not evaluate its parameter.

OK.

>> @@ -2814,6 +2818,7 @@ PP(pp_stat)
>>             goto do_fstat_have_io; 
>>         }
>> 
>> +	SvTAINTED_off(PL_statname); /* previous tainting irrelevant */
>> 	sv_setpv(PL_statname, SvPV_nomg_const_nolen(sv));
>> 	PL_statgv = NULL;
>> 	PL_laststype = PL_op->op_type;
>> 
>> @@ -1582,6 +1582,7 @@ Perl_do_readline(pTHX)
>> 		    if (av_len(GvAVn(PL_last_in_gv)) < 0) {
>> 			IoFLAGS(io) &= ~IOf_START;
>> 			do_open(PL_last_in_gv,"-",1,FALSE,O_RDONLY,0,NULL);
>> +			SvTAINTED_off(GvSVn(PL_last_in_gv)); /* previous tainting irrelevant */
>> 			sv_setpvs(GvSVn(PL_last_in_gv), "-");
>> 			SvSETMAGIC(GvSV(PL_last_in_gv));
>> 			fp = IoIFP(io);
> 
> The problem with adding a test for this is that before the patch nothing
> goes wrong and after the patch nothing goes wrong, so there's no regression
> to test.  I suppose I could write the test anyway, but I'd have to break
> Perl first, and then write a test targeted at a manually borken Perl.  This
> doesn't seem helpful.

OK.

> ----------------------------------------------------------------
> 
>> @@ -952,21 +1011,17 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
>>             else if (PL_compiling.cop_warnings == pWARN_ALL) {
>> 		/* Get the bit mask for $warnings::Bits{all}, because
>> 		 * it could have been extended by warnings::register */
>> -		HV * const bits=get_hv("warnings::Bits", 0);
>> -		if (bits) {
>> -		    SV ** const bits_all = hv_fetchs(bits, "all", FALSE);
>> -		    if (bits_all)
>> -			sv_setsv(sv, *bits_all);
>> -		}
>> -	        else {
>> -		    sv_setpvn(sv, WARN_ALLstring, WARNsize) ;
>> -		}
>> +		HV * const bits = get_hv("warnings::Bits", 0);
>> +		SV ** const bits_all = bits ? hv_fetchs(bits, "all", FALSE) : NULL;
>> +		if (bits_all)
>> +		    sv_copypv(sv, *bits_all);
>> +	        else
>> +		    sv_setpvn(sv, WARN_ALLstring, WARNsize);
>> 	    }
>>             else {
>> 	        sv_setpvn(sv, (char *) (PL_compiling.cop_warnings + 1),
>> 			  *PL_compiling.cop_warnings);
>> 	    }
>> -	    SvPOK_only(sv);
>> 	}
>> 	break;
>>     case '\015': /* $^MATCH */
>> 
>> I don’t see how this is related.  It might make things slightly faster, but it could go in a separate commit.
> 
> It's a vital change, though I can see why you missed it.  The old code
> didn't always set sv -- if you look carefully, there's a path where sv is
> not set at all.  With the magic flags patch, the semantics of a nop mg_get()
> have changed; now a failure to do anything leaves the previous value, rather
> than implicitly returning undef.  So this is necessary.

OK.

>> 
>> -	if (SvOK(sv) || SvGMAGICAL(sv)) {
>> -	    PL_ors_sv = newSVsv(sv);
>> -	}
>> -	else {
>> +	if (SvOK(sv))
>> +	    sv_copypv(PL_ors_sv = newSV(0), sv);
>> +	else
>> 	    PL_ors_sv = NULL;
>> -	}
>> 	break;
>> 
>> This looks like a bug fix.  Could we have a test, please, if it is?
> 
> Hm, it could be, but I'm not sure how to provoke it.  Perhaps assigning a
> tainted value to $\ ?

Or a tied value.

>> diff --git a/sv.c b/sv.c
>> index b96f7c1..dabaa04 100644
>> --- a/sv.c
>> +++ b/sv.c
>> @@ -2744,190 +2752,143 @@ Perl_sv_2pv_flags(pTHX_ register SV *const sv, STRLEN *const lp, const I32 flags
>> 	    *lp = 0;
>> 	return (char *)"";
>>     }
>> -    if (SvGMAGICAL(sv)) {
>> ...snipped pages and pages of diffiness...
>> +    if (SvIOK(sv)) {
>> 
>> Youch!  Did you really have to remove the if(SvTHINKFIRST) block?

You didn’t answer that question.

>> And even if you do remove it, you don’t have to reindent everything.  I really can’t see what has and hasn’t changed here.
>> 
> 
> I'm sorry but _yes_, if I remove an if, I _do_ have to reindent the code.
> Source control is not our master, it is our servant.  The code must not lie.
> Indentation must not lie.

Not every indentation level has to be four spaces.  You could re-ident just the outermost level.  If it really irks you to see eight spaces at one level, leave a bare block where the if was.

> And "diff -b" is your friend.

diff -b does not help in digging through history 1000 commits later to find out which commit changed something.  git blame is already annoyingly slow.  git blame -w is unusable.

> I'll save these for another patch, fine, but ... bleh.  The mix of coding
> styles is grating.  Impediments to understanding.

Oddly enough, I find code *easier* to understand when the style varies a bit.  It means if I come back to the code later I recognise it, because it does not all look the same.


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