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