Yes, that code looks like it handles ref counts correctly.

So wouldn't it be better if I do mortalize everything, and just SvREFCNT_inc whenever I either av_push() or hv_store() the SV, as well as using newRV_inc()? Will that not work and be correct?

Yes, I was coming to the same conclusion.

I hadn't bothered to grok this much about the ref-count nature of these Perlish macros before (I prefer very simple XS code for a lot of reasons). Now I see more why the Parrot designers think ref counting is so dang hard to get right. Perl's macros make it extra hard (and I found a memory leak in the Perl source already). I think this is mostly motivated by efficiency concerns, and I bet those concerns are overblown, but those are guesses on my part.

I think I'd much rather have a system where ref counts start at 0 and most things increment the ref count (including sv2mortal, av_push). That is, a system where an "extra" increment and decrement will often get done but you don't have to add things up and decide between *_inc and *_noinc and you don't have to consider inc-/decrementing ref counts directly.

Yes, you don't need to call SvREFCNT_inc() in the above code. You could choose to instead only create mortals and SvREFCNT_inc() each time you av_push() or such.

Sorry, I don't have FREETMPS grokked ATM and can't study that just now.

Someone asked about the Perl memory leak I spotted. Rather than try to put code into a /msg, I'll just include it here, that way anyone can point out that I'm mistaken. From line 3113 of op.c, Perl v5.8.0:

            repointer = newSViv(0);
            av_push(PL_regex_padav,SvREFCNT_inc(repointer));
[download]

newSViv() gives a ref count of 1. av_push() doesn't change ref count so the SvREFCNT_inc() leaves a ref count of 2 but repointer is not subsequently used so we've made one reference to it so we've got a memory leak.

I'm hoping the person who asked will perlbug this and save me the bother (not talking to p5p is always an advantage, IMO), but we'll see what happens. :)

- tye        

Okay! Last try, I hope. Does this look right?

void test(char *str)
{
    dSP;
    ENTER;
    SAVETMPS;
    PUSHMARK(SP);

    AV *arr = (AV *)sv_2mortal((SV *) newAV());
    SV *arr_value = sv_2mortal(newSVpv("test", 0));
    av_push( arr, arr_value);
    SvREFCNT_inc(arr_value);
    XPUSHs(sv_2mortal(newRV_inc((SV *)arr)));

    HV *hash = (HV *)sv_2mortal((SV *)newHV());
    SV *hash_value = sv_2mortal(newSVpv("test", 0));
    SvREFCNT_inc( hash_value );
    if ( NULL == hv_store( hash, "key", strlen("key"), hash_value, 0) 
+) {
        SvREFCNT_dec( hash_value );
        FREETMPS;
        LEAVE;
        return;
    }
    XPUSHs(sv_2mortal(newRV_inc((SV *)hash)));

    PUTBACK;
    call_pv("TestPM::test_leak", G_SCALAR);
    SPAGAIN;
    SV *retval = POPs;
    PUTBACK;
    FREETMPS;
    LEAVE;
}
[download]

Thanks again, tye.

--
edan

No it really doesnt look right to me. Have a look through the sources for similar code. I dont think youll see so many calls to sv_2mortal. I suggest using something like the XS that comes with DBI, or the XS that comes with List::Util for examples.

I realize that given my earlier mistaken reply you probably arent inclined to rate my opinion highly, but I strongly encourage you to look into this further. Its sufficiently different looking from XS and core code ive seen before that it rings alarm bells in my head. Especially given that you arent using newRV_noinc() anywhere, despite perlguts specifically saying you should be.

---
$world=~s/war/peace/g

I'd probably only SvREFCNT_inc() if the "insert" code succeeded. That also means that you don't need to SvREFCNT_dec().

I think you'd be better off not having to remember to FREETMPS; LEAVE; if you return(), so I'd go for a simpler XS sub that lets the framework insert such stuff for you and call a regular C sub that can return() when it feels like it and the calling XS sub will do the proper clean-up.

demerphq is surely correct that this style isn't typical. But I beleive it is correct in its handling of ref counts and also understand the motivation for the style.

- tye