Skip Menu |
 

This queue is for tickets about the Glib CPAN distribution.

Report information
The Basics
Id: 88533
Status: resolved
Priority: 0/
Queue: Glib

People
Owner: Nobody in particular
Requestors: 'spro^^*%*^6ut# [...] &$%*c
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: [PATCH] Possible fix for 5.19.4 compatibility
Download (untitled) / with headers
text/plain 844b
Yes, we’ve done it again. We’ve broken compatibility with Glib. :-) An implementation detail was leaking through. Passing any scalar to a function should make that scalar itself visible as $_[0], so foo($x) will cause \$_[0] to evaluate to the same thing as \$x. This did not work with undef. foo(undef) would make $_[0] refer to a brand new scalar, rather than the one that undef returns. Glib’s accumulator test in 7.t happens to depend on this bug. Or GType.xs does. In any case &PL_sv_undef is being passed as $_[1], and then 7.t expects to assign to it. Where &PL_sv_undef is coming from ultimately I don’t know, but the attached patch creates a brand new scalar in GType.xs, just before calling the accumulator callback, just the way perl used to do. All tests pass. I leave it to you to determine what is the best fix.
Subject: possible glib fix.txt
diff -rup Glib-1.301-gy1yG6/GType.xs Glib-1.301-8jML2z/GType.xs --- Glib-1.301-gy1yG6/GType.xs 2013-06-23 09:55:30.000000000 -0700 +++ Glib-1.301-8jML2z/GType.xs 2013-09-08 16:25:48.000000000 -0700 @@ -1091,7 +1091,10 @@ gperl_real_signal_accumulator (GSignalIn PUSHMARK (SP); PUSHs (sv_2mortal (newSVGSignalInvocationHint (ihint))); - SAVED_STACK_PUSHs (sv_2mortal (gperl_sv_from_value (return_accu))); + sv = gperl_sv_from_value (return_accu); + if (sv == &PL_sv_undef) + sv = newSV (0); + SAVED_STACK_PUSHs (sv_2mortal (sv)); SAVED_STACK_PUSHs (sv_2mortal (gperl_sv_from_value (handler_return))); if (callback->data)
RT-Send-CC: scott [...] asofyet.org
Download (untitled) / with headers
text/plain 1.4k
On Mon Sep 09 04:00:32 2013, SPROUT wrote: Show quoted text
> Yes, we’ve done it again. We’ve broken compatibility with Glib. :-)
No worries. Thanks for the report. Show quoted text
> An implementation detail was leaking through. Passing any scalar to a > function should make that scalar itself visible as $_[0], so foo($x) > will cause \$_[0] to evaluate to the same thing as \$x. > > This did not work with undef. foo(undef) would make $_[0] refer to a > brand new scalar, rather than the one that undef returns. > > Glib’s accumulator test in 7.t happens to depend on this bug. Or > GType.xs does. In any case &PL_sv_undef is being passed as $_[1], and > then 7.t expects to assign to it. Where &PL_sv_undef is coming from > ultimately I don’t know, but the attached patch creates a brand new > scalar in GType.xs, just before calling the accumulator callback, just > the way perl used to do. All tests pass.
Interesting. Upon inspection, I think t/7.t is at fault here. The undef appears in the accumulator callback's $_[1] because it is the first time the signal is called, hence no return values have been accumulated yet. The assigment directly to $_[1] appears to be simply a shortcut to avoid having to introduce a new lexical variable. As you say, this only worked by accident. The attached patch should fix this. muppet (CC), what do you think? Do you remember if the assignment to $_[1] in t/7.t line 97 is more than just a shortcut?
Subject: 0001-Tests-do-not-try-to-modify-a-read-only-value.patch
From d55cf959c30f30182cbed14d9f037735e6c5e6a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20Sch=C3=B6nfeld?= <kaffeetisch@gmx.de> Date: Tue, 10 Sep 2013 22:21:50 +0200 Subject: [PATCH] Tests: do not try to modify a read-only value t/7.t was failing on perl 5.19.4 because it attempted to assign to undef. --- t/7.t | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/t/7.t b/t/7.t index 6203725..1bee20e 100644 --- a/t/7.t +++ b/t/7.t @@ -84,23 +84,23 @@ use Glib::Object::Subclass flags => 'run-last', return_type => 'Glib::Scalar', accumulator => sub { - # the accumulator gets (ihint, return_accu, handler_return) + my ($ihint, $return_accu, $handler_return) = @_; # let's turn the return_accu into a list of all the handlers' # return values. this is weird, but the sort of thing you # might actually want to do. - print "# in accumulator, got $_[2], previously " - . (defined ($_[1]) ? $_[1] : 'undef') + print "# in accumulator, got $handler_return, previously " + . (defined ($return_accu) ? $return_accu : 'undef') . "\n"; - if ('ARRAY' eq ref $_[1]) { - push @{$_[1]}, $_[2]; + if ('ARRAY' eq ref $return_accu) { + push @{$return_accu}, $handler_return; } else { - $_[1] = [$_[2]]; + $return_accu = [$handler_return]; } # we must return two values --- a boolean that says whether # the signal keeps emitting, and the accumulated return value. # we'll stop emitting if the handler returns the magic # value 42. - ($_[2] != 42, $_[1]) + ($handler_return != 42, $return_accu) }, }, }, -- 1.8.1.2
RT-Send-CC: scott [...] asofyet.org
The fix has now been committed to the git repository. Thanks for your support.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.