The summary - I think that this change is correct and should stay.
The details - I did this. It's in blead. The commit message explains the
reasoning. The code removed is deadweight, and likely contains 2 (more)
branches, which are one of the banes of performant code:
commit 2517717a8902648de577d07f548caba6e3f2ecea
Author: Nicholas Clark <nick@ccl4.org>
Date: Mon Jul 19 07:01:15 2021 +0000
The cases for SVt_PVAV and SVt_PVHV in pp_defined are unreachable.
Remove them, and hit to the C compiler that it's unlikely that someone used
`defined` on a subroutine.
These have been unreachable since `defined @array` and `defined %hash`
became syntax errors. Whilst the same PP code is used for // and //=,
expressions such as`@a // @b` put the left array (or hash) in scalar
context, meaning that it always returns a define value.
(Should we warn on these?)
diff --git a/pp_hot.c b/pp_hot.c
index c693b301bb..fcc5e14454 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -1368,24 +1368,14 @@ PP(pp_defined)
}
defined = FALSE;
- switch (SvTYPE(sv)) {
- case SVt_PVAV:
- if (AvMAX(sv) >= 0 || SvGMAGICAL(sv) || (SvRMAGICAL(sv) && mg_find(sv, PERL_MAGIC_tied)))
- defined = TRUE;
- break;
- case SVt_PVHV:
- if (HvARRAY(sv) || SvGMAGICAL(sv) || (SvRMAGICAL(sv) && mg_find(sv, PERL_MAGIC_tied)))
- defined = TRUE;
- break;
- case SVt_PVCV:
+ if (UNLIKELY(SvTYPE(sv) == SVt_PVCV)) {
if (CvROOT(sv) || CvXSUB(sv))
defined = TRUE;
- break;
- default:
+ }
+ else {
SvGETMAGIC(sv);
if (SvOK(sv))
defined = TRUE;
- break;
}
if (is_dor) {
It turns out that it *is* reached by (at least) one testcase on CPAN:
https://metacpan.org/release/SYBER/XS-Framework-1.5.0/source/t/upgrade.t#L14
# undefs to HV
$obj = bless \(my $a = undef), 'AAA';
XS::Framework::obj2hv($obj);
ok($obj->{key} = 1);
is($obj->{key}, 1);
ok defined $a;
Note, at this point, $a *isn't* a scalar. It's a hash. *Not* a reference to
a hash. An actual hash.
If I add:
use Devel::Peek; Dump ($a);
my $copy = $a;
I get this:
$ /home/nick/Sandpit/snap-v5.35.2-247-gc243917ec3-g/bin/perl5.35.3 -Mblib t/upgrade.t
ok 1
ok 2
not ok 3
# Failed test at t/upgrade.t line 14.
SV = PVHV(0x562e3f5f8fa0) at 0x562e3f706ff8
REFCNT = 2
FLAGS = (OBJECT,SHAREKEYS)
STASH = 0x562e3f60faf0 "AAA"
ARRAY = 0x562e3f625db0 (0:7, 1:1)
hash quality = 100.0%
KEYS = 1
FILL = 1
MAX = 7
Elt "key" HASH = 0x2f975654
SV = IV(0x562e3f60fb88) at 0x562e3f60fb98
REFCNT = 1
FLAGS = (IOK,pIOK)
IV = 1
Bizarre copy of HASH in scalar assignment at t/upgrade.t line 16.
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 3.
Given that both:
1) You can only create this situation from XS code
2) What is created is *not* legal in Perl-space
I don't think that what I changed was "supported" behaviour (or even
"specified").
Hence I think that the behaviour in blead should stay, and the distribution
should change this line in this regression test, to express its testing
intent in some other way.
I'm not quite sure *what* the defined test is needed for, as there's this
test for `defined $a` after the call to obj2hv() but no corresponding call
after obj2av(). Maybe it should just go. Otherwise, taking a reference to $a
and then using Scalar::Util::reftype on that exposes the storage type
(which changes from 'SCALAR' to 'HASH') - ie:
$ diff -u t/upgrade.t.orig t/upgrade.t
--- t/upgrade.t.orig 2021-07-13 11:02:17.000000000 +0000
+++ t/upgrade.t 2021-08-19 13:42:54.401902362 +0000
@@ -3,15 +3,17 @@
use Test::More;
use Test::Deep;
use XS::Framework;
+use Scalar::Util qw(reftype);
my $obj;
# undefs to HV
$obj = bless \(my $a = undef), 'AAA';
+is(reftype(\$a), 'SCALAR');
XS::Framework::obj2hv($obj);
ok($obj->{key} = 1);
is($obj->{key}, 1);
-ok defined $a;
+is(reftype(\$a), 'HASH');
XS::Framework::obj2hv($obj);
Nicholas Clark
The details - I did this. It's in blead. The commit message explains the
reasoning. The code removed is deadweight, and likely contains 2 (more)
branches, which are one of the banes of performant code:
commit 2517717a8902648de577d07f548caba6e3f2ecea
Author: Nicholas Clark <nick@ccl4.org>
Date: Mon Jul 19 07:01:15 2021 +0000
The cases for SVt_PVAV and SVt_PVHV in pp_defined are unreachable.
Remove them, and hit to the C compiler that it's unlikely that someone used
`defined` on a subroutine.
These have been unreachable since `defined @array` and `defined %hash`
became syntax errors. Whilst the same PP code is used for // and //=,
expressions such as`@a // @b` put the left array (or hash) in scalar
context, meaning that it always returns a define value.
(Should we warn on these?)
diff --git a/pp_hot.c b/pp_hot.c
index c693b301bb..fcc5e14454 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -1368,24 +1368,14 @@ PP(pp_defined)
}
defined = FALSE;
- switch (SvTYPE(sv)) {
- case SVt_PVAV:
- if (AvMAX(sv) >= 0 || SvGMAGICAL(sv) || (SvRMAGICAL(sv) && mg_find(sv, PERL_MAGIC_tied)))
- defined = TRUE;
- break;
- case SVt_PVHV:
- if (HvARRAY(sv) || SvGMAGICAL(sv) || (SvRMAGICAL(sv) && mg_find(sv, PERL_MAGIC_tied)))
- defined = TRUE;
- break;
- case SVt_PVCV:
+ if (UNLIKELY(SvTYPE(sv) == SVt_PVCV)) {
if (CvROOT(sv) || CvXSUB(sv))
defined = TRUE;
- break;
- default:
+ }
+ else {
SvGETMAGIC(sv);
if (SvOK(sv))
defined = TRUE;
- break;
}
if (is_dor) {
It turns out that it *is* reached by (at least) one testcase on CPAN:
https://metacpan.org/release/SYBER/XS-Framework-1.5.0/source/t/upgrade.t#L14
# undefs to HV
$obj = bless \(my $a = undef), 'AAA';
XS::Framework::obj2hv($obj);
ok($obj->{key} = 1);
is($obj->{key}, 1);
ok defined $a;
Note, at this point, $a *isn't* a scalar. It's a hash. *Not* a reference to
a hash. An actual hash.
If I add:
use Devel::Peek; Dump ($a);
my $copy = $a;
I get this:
$ /home/nick/Sandpit/snap-v5.35.2-247-gc243917ec3-g/bin/perl5.35.3 -Mblib t/upgrade.t
ok 1
ok 2
not ok 3
# Failed test at t/upgrade.t line 14.
SV = PVHV(0x562e3f5f8fa0) at 0x562e3f706ff8
REFCNT = 2
FLAGS = (OBJECT,SHAREKEYS)
STASH = 0x562e3f60faf0 "AAA"
ARRAY = 0x562e3f625db0 (0:7, 1:1)
hash quality = 100.0%
KEYS = 1
FILL = 1
MAX = 7
Elt "key" HASH = 0x2f975654
SV = IV(0x562e3f60fb88) at 0x562e3f60fb98
REFCNT = 1
FLAGS = (IOK,pIOK)
IV = 1
Bizarre copy of HASH in scalar assignment at t/upgrade.t line 16.
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 3.
Given that both:
1) You can only create this situation from XS code
2) What is created is *not* legal in Perl-space
I don't think that what I changed was "supported" behaviour (or even
"specified").
Hence I think that the behaviour in blead should stay, and the distribution
should change this line in this regression test, to express its testing
intent in some other way.
I'm not quite sure *what* the defined test is needed for, as there's this
test for `defined $a` after the call to obj2hv() but no corresponding call
after obj2av(). Maybe it should just go. Otherwise, taking a reference to $a
and then using Scalar::Util::reftype on that exposes the storage type
(which changes from 'SCALAR' to 'HASH') - ie:
$ diff -u t/upgrade.t.orig t/upgrade.t
--- t/upgrade.t.orig 2021-07-13 11:02:17.000000000 +0000
+++ t/upgrade.t 2021-08-19 13:42:54.401902362 +0000
@@ -3,15 +3,17 @@
use Test::More;
use Test::Deep;
use XS::Framework;
+use Scalar::Util qw(reftype);
my $obj;
# undefs to HV
$obj = bless \(my $a = undef), 'AAA';
+is(reftype(\$a), 'SCALAR');
XS::Framework::obj2hv($obj);
ok($obj->{key} = 1);
is($obj->{key}, 1);
-ok defined $a;
+is(reftype(\$a), 'HASH');
XS::Framework::obj2hv($obj);
Nicholas Clark