p5pclub / ref-util

Ref::Util - Utility functions for checking references
6 stars 11 forks source link

Regexp check is wrong for older perls #9

Closed xsawyerx closed 8 years ago

xsawyerx commented 8 years ago

Reported by Yves Orton.

It should return true for is_regexpref(bless qr/foo/, foo), and as written it wouldn't. You chould look at code in Data::Dump::Streamer, see http://cpansearch.perl.org/src/YVES/Data-Dump-Streamer-2.39/lib/Data/Dump/Streamer.xs

Search for: mg_find(sv, PERL_MAGIC_qr) Or you could look at the log for universal.c in perl, where in 803059618a6e90fb614193e8cdf81c79f27d8764 I added this:

  +regexp *
  +Perl_get_re_arg( pTHX_ SV *sv, U32 flags, MAGIC **mgp) {
  +    MAGIC *mg;
  +    if (sv) {
  +        if (SvMAGICAL(sv))
  +            mg_get(sv);
  +        if (SvROK(sv) &&
  +            (sv = (SV*)SvRV(sv)) &&     /* assign deliberate */
  +            SvTYPE(sv) == SVt_PVMG &&
  +            (mg = mg_find(sv, PERL_MAGIC_qr))) /* assign deliberate */
  +        {       
  +            if (mgp) *mgp = mg;
  +            return (regexp *)mg->mg_obj;      
  +        }
  +    }   
  +    if (mgp) *mgp = NULL;
  +    return ((flags && PL_curpm) ? PM_GETRE(PL_curpm) : NULL);
  +}

which contains pretty much what you want. BTW, re.pm has an is_regexp function, perhaps you should test yours against it. Dont forget to test blessed regexps.

demerphq commented 8 years ago

Probably the docs should also mention re::is_regexp(), which has one nice feature of always being available in modern Perls.

xsawyerx commented 8 years ago

Do you think is_regexpref() should just delegate to re::is_regexp()?

demerphq commented 8 years ago

On 7 March 2016 at 11:22, Sawyer X notifications@github.com wrote:

Do you think is_regexpref() should just delegate to re::is_regexp()?

Not necessarily. I think maybe we should rename it while the module is new and fresh. :-)

The modern implementation is this:

XS(XS_re_is_regexp); /* prototype to pass -Wmissing-prototypes */ XS(XS_re_is_regexp) { dXSARGS; PERL_UNUSED_VAR(cv);

if (items != 1)
    croak_xs_usage(cv, "sv");

if (SvRXOK(ST(0))) {
    XSRETURN_YES;
} else {
    XSRETURN_NO;
}

}

Which IMO is superior to the current implementation of just checking the reftype, even if that is all that SvRXOK() does. I see no problem with Ref::Util exporting its own copy with the same code.

Yves

xsawyerx commented 8 years ago

In older perls the regexp was not a reference, basically?

demerphq commented 8 years ago

On 7 March 2016 at 11:48, Sawyer X notifications@github.com wrote:

In older perls the regexp was not a reference, basically?

Regexp's have always been some form of blessed reference. In older perls the regexp struct was attached to the ref via magic. BTW, you have to understand the history involved here, in really old perls regexp's were not perl visible objects, they were attributes of the opcode that executed a regexp. later on qr// was invented, which turned the previously private regexp objects into public vars that could be passsed around. It took quite a lot of iterations (and bug fixes) to move from "blessed SCALAR with magic" to the current "full blown SV sub-type".

Over various perls the exact details changed several times, with the biggest change being when we switched to being a full blown "sv type", and even that has had several iteration made to the finer details.

The sub I quoted was a first step to producing standardized code to decide if a scalar was a regexp, which then lead to the RXOK macro.

So assuming you want to go back to 5.8 I suggest you use something I quoted, but if at all possible use core routines for it.

Yves

xsawyerx commented 8 years ago

@demerphq We've changed the regexp check to use SvRXOK for 5.10 and above (where it's available, basically) and we're working with @wolfsage to make that available for previous versions.

xsawyerx commented 8 years ago

We resolved this in the previous version.