On Fri, Jan 6, 2012 at 06:34, Andrew Dunstan <andrew@dunslane.net> wrote:
>> PFA that copies if its readonly and its not a scalar.
>>
>> I didn't bother adding regression tests-- should I have?
> I have several questions.
>
> 1. How much are we actually saving here? newSVsv() ought to be pretty cheap,
> no? I imagine it's pretty heavily used inside the interpreter.
It will incur an extra copy for every return value from plperl and
every value passed to a spi function (and possibly other areas I
forgot about). Personally I think avoiding yet another copy of the
return value is worth it.
> 2. Unless I'm insufficiently caffeinated right now, there's something wrong
> with this logic:
>
> ! if (SvREADONLY(sv) &&
> ! (type != SVt_IV ||
> ! type != SVt_NV ||
> ! type != SVt_PV))
Oh my... I dunno exactly what I was smoking last night, but its a good
thing I didn't share :-). Uh so my test program was also completely
wrong, Ill have to redo it all. I've narrowed it down to: if ((type == SVt_PVGV || SvREADONLY(sv))) {
if (type != SVt_PV && type != SVt_NV) { sv = newSVsv(sv); } }
One odd thing is we have to copy SVt_IV (plain numbers) as apparently
that is shared with refs (my $str = 's'; my $ref = \$str;).
Given this, #3 and the other unknowns we might run into in the future
I think if ((SvREADONLY(sv) || type == SVt_GVPV) proposed upthread
makes the most sense.
> 3. The above is in any case almost certainly insufficient, because in my
> tests a typeglob didn't trigger SvREADONLY(), but did cause a crash.
Hrm the glob I was testing was *STDIN. It failed to fail in my test
program because I was not testing *STDIN directly but instead had
@test = (*STDIN); Ugh. Playing with it a bit more it seems only
*STDIN, *STDERR and *STDOUT have problems. For example this seems to
work fine for me:
do LANGUAGE plperlu $$ open(my $fh, '>', '/tmp/t.tmp'); elog(NOTICE, $fh) $$;
> And yes, we should possibly add a regression test or two. Of course, we
> can't use the cause of the original complaint ($^V) in them, though.
I poked around the perl source looking for some candidates elog(INFO,
${^TAINT}) works fine on 5.8.9 and 5.14.2. I thought we could get away
with elog(INFO, *STDIN) but 5.8.9 says:
NOTICE:
While other version of perl (5.14) say:
NOTICE: *main::STDIN
