_is_arrayref, _is_hashref and _is_coderef is incorrectly defined

[pali]

See implementation: https://github.com/Tux/Text-CSV_XS/blob/master/CSV_XS.xs#L60:

#define _is_arrayref(f) ( f && \
     (SvROK (f) || (SvRMAGICAL (f) && (mg_get (f), 1) && SvROK (f))) && \
      SvOK (f) && SvTYPE (SvRV (f)) == SVt_PVAV )
#define _is_hashref(f) ( f && \
     (SvROK (f) || (SvRMAGICAL (f) && (mg_get (f), 1) && SvROK (f))) && \
      SvOK (f) && SvTYPE (SvRV (f)) == SVt_PVHV )
#define _is_coderef(f) ( f && \
     (SvROK (f) || (SvRMAGICAL (f) && (mg_get (f), 1) && SvROK (f))) && \
      SvOK (f) && SvTYPE (SvRV (f)) == SVt_PVCV )

Code first checks for SvRMAGICAL() and then calls mg_get(). But SvRMAGICAL checks for SVs_RMG -- magic different from get/set, in most cases uses for clear function. So it does not make sense to call mg_get() method based on SVs_RMG result.

Instead SvRMAGICAL() there should be used SvGMAGICAL(), check for SVs_GMG that scalar has get magic which means that mg_get needs to be called.

To simplify code I would propose to use SvGETMAGIC() macro which calls mg_get() when it is needed. E.g. _is_arrayref(f) could looks like this:

static inline bool _is_arrayref(SV *sv) {
    if (!sv) return false;
    SvGETMAGICAL(sv);
    if (!SvROK(sv)) return false;
    if (SvTYPE(SvRV(sv)) != SVt_PVAV) return false;
    return true;
}

[Tux]

Can you write that as a macro? As inline is not supported on all compilers, and I'd hate to make those slow just because gcc can do a better job

[Tux]

@pali do you have a test case that fails under the current code?

[pali]

For test case you need to take scalar which do not have SVs_RMG, but has SVs_GMG. Then mg_get() would never be called.

[pali]

Can you write that as a macro?

Yes, just rewrite commands via standard boolean logic into one if statement. E.g.:

#define _is_arrayref(sv) ((sv) && (SvGETMAGICAL(sv), 1) && (SvROK(sv)) && (SvTYPE(SvRV(sv)) == SVt_PVAV))

[Tux]

#define _is_reftype(f,x) \
    (f && (SvGETMAGIC (f), 1) && SvROK (f) && SvTYPE (SvRV (f)) == x)
#define _is_arrayref(f) _is_reftype (f, SVt_PVAV)
#define _is_hashref(f)  _is_reftype (f, SVt_PVHV)
#define _is_coderef(f)  _is_reftype (f, SVt_PVCV)

Still passed all tests. I could not come up with a new test in t/76_magic.t to probe the old code was wrong, but this looks cleaner anyway.
BTW it is SvGETMAGIC (sv) not SvGETMAGICAL (sv).
See 0fafcee

[pali]

For example magic variable $$ has GMG and does not have RMG. But it is not reference. I do not know currently which internal perl variable is magic, reference and also without RMG.

[Tux]

Sure, but I still could not (yet) come up with a test :)

[Tux]

Change passed all tests on all 200+ perl versions I have that should be supported and all 858 modules on CPAN that directly or indirectly depend on my still pass. The speed test did not show a slowdown.
Just have to check on the other architectures, and it will be on its way to CPAN.
Thanks again for your input and feedback.

[Tux]

Released in 1.37. Closing