develooper Front page | perl.perl5.porters | Postings from September 2013

Re: [perl #119251] [PATCH] Enhancement request - a prototype attribute

Thread Next
From:
Peter Martini
Date:
September 17, 2013 06:09
Subject:
Re: [perl #119251] [PATCH] Enhancement request - a prototype attribute
Message ID:
CAFyW6MRH0Y=KuZEZ9++BKs-RGUoYPUn-YsmtmSscrXw4Zoukpw@mail.gmail.com
On Wed, Aug 14, 2013 at 2:54 AM, Father Chrysostomos via RT
<perlbug-followup@perl.org> wrote:
> On Tue Aug 13 20:34:07 2013, pcm wrote:
>> Attaching two new patches after addressing the bad merge and the lack of a
>> check for a closing parens.
>
> Sorry for not doing this all at once, but I have only just scrutinized
> your patches closely, probably more closely than you would like. :-)
>
> First patch:
>
> Good enough for application, but I have suggestions for improvement:
>
> +                   if (hek) {
> +                       subname = sv_2mortal(newSVhek(hek));
> +                   }
> +                   else {
> +                       subname = sv_newmortal();
> +                       gv_efullname3(subname, CvGV((const CV *)sv), NULL);
> +                   }
> +                   Perl_validate_proto(aTHX_ subname, proto, ckWARN(WARN_ILLEGALPROTO));
> +                   Perl_cv_ckproto_len_flags(aTHX_ (const CV *)sv, (const GV
> *)subname, name+10, len-11, SvUTF8(attr));
>
> You can avoid creating the subname SV altogether by checking the warning
> settings before the sub calls.  If only cv_ckproto_len_flags is going to
> warn, you can avoid stringifying the GV, as cv_ckproto_len_flags takes a GV.
>
> Also, in general, if Perl_validate_proto’s return value is being
> discarded, then you don’t even need to call it and validate the
> prototype if the third argument is false.

Attached a new patch with those changes.

>
> Y’know, that also applies to the existing call in toke.c.
>
> Second patch:
>
> $ ./miniperl -e 'sub f { use }'
> Assertion failed: (name), function S_move_proto_attr, file op.c, line 2578.
> Abort trap: 6

I'm getting:

[pmartini@pmlenlaptop blah]$ ./miniperl -e 'sub f { use }'
syntax error at -e line 1, near "use }"
Execution of -e aborted due to compilation errors.

If you apply the original second patch now, do you still see the error?

>
> +STATIC void
> +S_move_proto_attr(pTHX_ OP **proto, OP **attrs, const GV * name)
> +{
> ...
> +        if (pvlen >= 10 && memEQ(pv, "prototype(", 10)) {
> +            new_proto = newSVOP(OP_CONST, o->op_flags,
> +                            newSVpvn_flags(pv+10, pvlen - 11,
> SvUTF8(cSVOPo_sv)));
> +            op_free(o);
> +            *attrs = NULL;
> +        }
>
> Avoid reallocating stuff:
>
> SvREADONLY_off(cSVOPo_sv);
> /* SvPV_force should be a no-op most of the time, so
>    this optimises for the most common case */
> SvPV_force_nolen(cSVOPo_sv);
> sv_chop(cSVOPo_sv, pv+10);
> SvCUR(cSVOPo)--;
> SvEND(cSVOPo)='\0';
> new_proto = o;
> *attrs = NULL;
>
> That gets a bit messy, even if it is faster.  Maybe this is better:
>
> SV * const new_sv = newSVpvn_flags(pv+10, pvlen - 11, SvUTF8(cSVOPo_sv));
> SvREFCNT_dec(cSVOPo_sv);
> cSVOPo_sv = new_sv;
> new_proto = o
> *attrs = NULL;

I'd originally done it the first way you'd suggested, and had backed
off because it was too messy.  Just creating the new SV sounds good,
I'll do that.  (I don't recall any longer why I created a whole new op
instead of just the SV; I think just excessive paranoia).

>
> +    } else if (o->op_type == OP_LIST) {
> +        OP *rop = NULL;
> +        assert(o->op_flags && OPf_KIDS);
> +        for (o = cLISTOPo->op_first; o; o=o->op_sibling) {
> +            if (o->op_type == OP_CONST) {
> +                pv = SvPV(cSVOPo_sv, pvlen);
> +                if (pvlen >= 10 && memEQ(pv, "prototype(", 10)) {
> +                    new_proto = newSVOP(OP_CONST, o->op_flags,
> +                        newSVpvn_flags(pv+10, pvlen - 11,
> SvUTF8(cSVOPo_sv)));
> +                }
> +                else {
> +                    rop = op_append_elem(OP_LIST, rop,
> +                        newSVOP(OP_CONST, o->op_flags,
> +                        SvREFCNT_inc_NN(cSVOPo->op_sv)));
> +                }
> +            }
> +        }
> +        op_free(*attrs);
> +        *attrs = rop;
> +    }
>
> Instead of reconstructing the list like that, you could just splice the
> prototype attribute out of it, something like this:
>
> OP **too = &cLISTOPo->op_first;
> for(...) {
>     ...
>     if(...) {
>         *too = o->op_sibling;
>         ...
>     }
>     too = &o->op_sibling;
> }

Ok, will do.

>
> Also, you are leaking an op+sv if there are multiple prototype()
> thingies in the attrlist:
>
> $ ./perl -Ilib -MXS::APItest -le 'for(1..10) { eval q"+sub :prototype($)
> :prototype($$) {}"; print sv_count; }'
> 3959
> 3960
> 3961
> 3962
> 3963
> 3964
> 3965
> 3966
> 3967
> 3968
>
> Also, why is that not warning about a mismatch?  Or maybe just warn
> about duplicate prototype attributes, to keep the code simpler.  Either
> way, it should not leak but should probably still be accepted.

Good catch, I'll update it to warn on multiple declarations, and of
course fix the leak.

>
> +        (void)validate_proto(svname, cSVOPx_sv(new_proto),
> ckWARN(WARN_ILLEGALPROTO));
>
> Again, check if(ckWARN...) and avoid creating a new SV otherwise.
>
>

Ok

> --
>
> Father Chrysostomos
>

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