New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AUTOLOAD is not called when DESTROY is missing in 5.20.1 - but this is not documented #14673
Comments
From @szabgabIn earlier versions of perl if AUTOLOAD is defined in a module but DESTROY In 5.20.1 this does not seem to be the case. In 5.20.1 it seems if AUTOLOAD However I think the documentation in perlobj still indicates the old So I wonder if this was an unplanned change in perl, The issue was discussed here as well: http://www.perlmonks.org/?node_id=1124504 |
From @iabynOn Fri, Apr 24, 2015 at 11:16:23PM -0700, Gabor Szabo wrote:
The change bisects to this, which leads to the suspicion that it was commit 8c34e50 [perl #114864] Don’t use amt for DESTROY -- |
The RT System itself - Status changed from 'new' to 'open' |
From @rjbs* Dave Mitchell <davem@iabyn.com> [2015-04-25T06:25:34]
That does look unintended. I don't see discussion of the change on p5p or I know I've added special casing to AUTOLOADs to skip DESTROY, which makes me The downside is breaking code only ever written or tested on 5.18.0, or later. -- |
From @ikegamiOn Sat, Apr 25, 2015 at 2:16 AM, Gabor Szabo <perlbug-followup@perl.org>
Ow! The bug isn't the documentation; it's that AUTOLOAD isn't being called |
From @VadimPushtaevThe perldoc says: If you define an AUTOLOAD in your class, then Perl will call your AUTOLOAD to handle the DESTROY method. You can prevent this by defining an empty DESTROY, like we did in the autoloading example. You can also check the value of $AUTOLOAD and return without doing anything when called to handle DESTROY . However, my AUTOLOAD is not called in some versions of Perl (including the current stable 5.22.0). Here is what I tried: # 5.8.8 # 5.14.2 # 5.18.2 # 5.20.2 # 5.22.0 I'm not sure whether it's a documentation issue or it should not behave this way at all. |
From @tonycozOn Tue Oct 20 02:25:43 2015, pushtaev.vm@gmail.com wrote:
Looks like this changed in 8c34e50. Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @VadimPushtaevHello, thanks for you reply! I googled that commmit identifier you provided and found this: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=124387 Looks like it has already been reported and isn't fixed yet. Is there a way I mark this ticket as duplicate or something? -- On Tue Oct 20 18:42:26 2015, tonyc wrote:
|
From @tonycozOn Thu Oct 22 01:56:47 2015, pushtaev.vm@gmail.com wrote:
Thanks, I've merged the tickets. Tony |
From @toddr
That makes this sound related to https://rt.perl.org/Ticket/Display.html?id=126410 <https://rt-archive.perl.org/perl5/Ticket/Display.html?id=126410>. Which is the third time (overload changes also broke B::C in bee7c57) this week I've been fighting with these sets of changes introduced in 5.17. As I look at the commits, I'm forced to ask why we would want these speedup improvements when the commit messages say: 10% increase in stash lookup ... Other stash lookups will have other overheads that make the difference less impressive. So basically it's a 10% speedup on trivial cases. Which were probably billisecond improvements to begin with? I promise I'm not trying to trivialize the effort that was put in. It seems like we changed some fundamental things about how Perl checks and uses some of the magic. However in doing so made the code potentially more complex and introduced several bugs we're now starting to notice. It's possible I'm misunderstanding these changes and they're not as related as they seem. However if they are, I'm wondering if it's worth reverting the improvements to fix these problems? I admit that at this point it may be more complex than some other approach. Todd. |
From @tonycozOn Fri Apr 24 23:16:23 2015, gabor@szabgab.com wrote:
The attached, along with the two patches on #126410, fixes this for me. I needed to change one of the existing tests in op/method.t to allow for DESTROY being called on the stdio objects. I don't think these patches can be applied as-is to maint, though a different patch could fix this problem in maint. Tony |
From @tonycoz0003-perl-124387-TODO-test-for-AUTOLOAD-on-DESTROY.patchFrom 8e91b8fcc8f1c911ccd80dc5bef1639a8a51d2f4 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 19 Jan 2016 11:39:48 +1100
Subject: [perl #124387] TODO test for AUTOLOAD on DESTROY
---
t/op/method.t | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/t/op/method.t b/t/op/method.t
index 0d7f254..bdac5bb 100644
--- a/t/op/method.t
+++ b/t/op/method.t
@@ -13,7 +13,7 @@ BEGIN {
use strict;
no warnings 'once';
-plan(tests => 148);
+plan(tests => 149);
@A::ISA = 'B';
@B::ISA = 'C';
@@ -460,6 +460,17 @@ is $kalled, 1, 'calling a class method via a magic variable';
{ bless {}, "NoSub"; }
}
+{
+ # [perl #124387]
+ local $::TODO = "AUTOLOAD not being called for DESTROY";
+ my $autoloaded;
+ package AutoloadDestroy;
+ sub AUTOLOAD { $autoloaded = 1 }
+ package main;
+ bless {}, "AutoloadDestroy";
+ ok($autoloaded, "AUTOLOAD called for DESTROY");
+}
+
eval { () = 3; new {} };
like $@,
qr/^Can't call method "new" without a package or object reference/,
--
2.1.4
|
From @tonycoz0004-perl-124387-call-AUTOLOAD-when-DESTROY-isn-t-defined.patchFrom 4e3b4ec09e4002c0b74eca141e662b1403d65aaa Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 19 Jan 2016 11:42:21 +1100
Subject: [perl #124387] call AUTOLOAD when DESTROY isn't defined
---
sv.c | 3 ++-
t/op/method.t | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/sv.c b/sv.c
index 0a5ffe8..9c4fed6 100644
--- a/sv.c
+++ b/sv.c
@@ -6738,6 +6738,7 @@ S_curse(pTHX_ SV * const sv, const bool check_refcnt) {
if (HvNAME(stash)) {
CV* destructor = NULL;
struct mro_meta *meta;
+
assert (SvOOK(stash));
DEBUG_o( deb(aTHX_ "Looking for DESTROY method for %s\n",
@@ -6753,7 +6754,7 @@ S_curse(pTHX_ SV * const sv, const bool check_refcnt) {
}
else {
GV * const gv =
- gv_fetchmeth_autoload(stash, "DESTROY", 7, 0);
+ gv_fetchmethod_pvn_flags(stash, "DESTROY", 7, GV_AUTOLOAD);
if (gv) destructor = GvCV(gv);
HvMROMETA(stash)->destroy_gen = PL_sub_generation;
HvMROMETA(stash)->destroy = destructor;
diff --git a/t/op/method.t b/t/op/method.t
index bdac5bb..d11a703 100644
--- a/t/op/method.t
+++ b/t/op/method.t
@@ -361,6 +361,7 @@ for my $meth (['Bar', 'Foo::Bar'],
{
fresh_perl_is(<<EOT,
package UNIVERSAL; sub AUTOLOAD { my \$c = shift; print "\$c \$AUTOLOAD\\n" }
+sub DESTROY {} # prevent AUTOLOAD being called on DESTROY
package Xyz;
package main; Foo->$meth->[0]();
EOT
@@ -462,7 +463,6 @@ is $kalled, 1, 'calling a class method via a magic variable';
{
# [perl #124387]
- local $::TODO = "AUTOLOAD not being called for DESTROY";
my $autoloaded;
package AutoloadDestroy;
sub AUTOLOAD { $autoloaded = 1 }
--
2.1.4
|
From @tonycozOn Mon Jan 18 16:53:50 2016, tonyc wrote:
Applied as f05081b and 000814d. Tony |
@tonycoz - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for submitting this report. You have helped make Perl better. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0 |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#124387 (status was 'resolved')
Searchable as RT124387$
The text was updated successfully, but these errors were encountered: