Bug #125645 for Class-XSConstructor: Useless creation of a variable

Wed Jun 20 20:55:08 2018 bkb [...] cpan.org - Ticket created

Subject:

Useless creation of a variable

Here, https://metacpan.org/source/TOBYINK/Class-XSConstructor-0.007/XSConstructor.xs#L94 what's the point of creating val3? You've already copied the variable into val2, then you create another instance of the same variable, then you mark it to have its reference count decremented. Why not just use the existing val2 and not decrement the refcount? Dist::Zilla doesn't work for me so I was unable to try it on your repository, but editing the code from the distribution, using val2 seems not to create problems. This is the code from version 0.006 which I downloaded yesterday: if (hv_exists(args, keyname, keylen)) { SV** val = hv_fetch(args, keyname, keylen, 0); SV* val2 = newSVsv(*val); /* optional isa check */ if (hv_exists(ISA_attrs, keyname, keylen)) { SV** const check = hv_fetch(ISA_attrs, keyname, keylen, 0); SV* result; dSP; int count; ENTER; SAVETMPS; PUSHMARK(SP); EXTEND(SP, 1); PUSHs(val2); PUTBACK; count = call_sv(*check, G_SCALAR); result = POPs;

Thu Jun 21 09:03:28 2018 perl [...] toby.ink - Correspondence added

That part of the code, I was having a lot of problems with nulls getting passed to the callback sub, perhaps because they'd been garbage collected or something. I'm pretty sure I need at least one copy of the variable to ensure that the callback code can't assign to @_ and alter the value. Two copies is probably unnecessary, but it was the only way I could get things to work reliably. I'd welcome any patches to make this more efficient though.

Thu Jun 21 09:03:29 2018 The RT System itself - Status changed from 'new' to 'open'

Thu Jun 21 20:04:54 2018 benkasminbullock [...] gmail.com - Correspondence added

CC:	BKB <bkb [...] cpan.org>
Subject:	Re: [rt.cpan.org #125645] Useless creation of a variable
Date:	Fri, 22 Jun 2018 09:03:41 +0900
To:	bug-Class-XSConstructor [...] rt.cpan.org
From:	Ben Bullock <benkasminbullock [...] gmail.com>

On 21 June 2018 at 22:03, Toby Inkster via RT <bug-Class-XSConstructor@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125645 > > > That part of the code, I was having a lot of problems with nulls getting passed to the callback sub, perhaps because they'd been garbage collected or something. I'm pretty sure I need at least one copy of the variable to ensure that the callback code can't assign to @_ and alter the value. Two copies is probably unnecessary, but it was the only way I could get things to work reliably. >

An SV can't be altered by altering the contents of the subroutine calling it, because it's the contents of an $x variable, not the $x variable itself. To demonstrate this I created an example module. Here is your code in an example format: https://github.com/benkasminbullock/toby-xs/blob/master/XS.xs Here is the test: https://github.com/benkasminbullock/toby-xs/blob/master/t/toby-xs.t The variable is set here: https://github.com/benkasminbullock/toby-xs/blob/master/t/toby-xs.t#L8 Then it is altered here: https://github.com/benkasminbullock/toby-xs/blob/master/t/toby-xs.t#L12 Then we can check it is not altered here: https://github.com/benkasminbullock/toby-xs/blob/master/t/toby-xs.t#L16 You can increment the refcount: https://github.com/benkasminbullock/toby-xs/blob/master/XS.xs#L20 Show quoted text

> I'd welcome any patches to make this more efficient though.

For some reason Dist::Zilla does not work for me so I can't install your module from the github, however I hope this is helpful. Please feel free to close the issue if it is no longer relevant. I'll probably delete the above example module in a week or so, unless you object.

Fri Jun 22 05:04:44 2018 perl [...] toby.ink - Correspondence added

On 2018-06-22T01:04:54+01:00, benkasminbullock@gmail.com wrote: Show quoted text

> An SV can't be altered by altering the contents of the subroutine > calling it, because it's the contents of an $x variable, not the $x > variable itself.

Try changing the callback sub in the test case to: my $sub = sub { $_[0] .= 'monkeys'; return $_[0]; }; No $thang variable. You'll see what I mean.

Fri Jun 22 05:07:13 2018 perl [...] toby.ink - Correspondence added

Also, I don't use Dist::Zilla. I use my own dist builder, Dist::Inkt. It has a lot of deps so you may be better off just using the tarballs from CPAN.

Tue Jul 24 20:39:34 2018 perl [...] toby.ink - Status changed from 'open' to 'rejected'