perlbug-followup at perl
Apr 28, 2012, 5:41 PM
Post #9 of 20
(161 views)
Permalink
|
On Tue Apr 24 18:03:20 2012, sprout wrote:
> On Sun Apr 22 18:54:29 2012, jkeenan wrote:
> > On Sun Apr 22 03:54:42 2012, demerphq wrote:
> > >
> > > And/or add hash_locked() as a negation of hash_unlocked()
> > >
> > > Yves
> >
> >
> > Please review the patch attached.
>
> Looks good. I see that you fixed hash_unlocked, which was giving a
> negated answer.
>
> I’ve pushed these to the sprout/misc-post-5.16 branch for now.
>
Summary: The more I look at Hash::Util, the more I think it is wrong.
In https://rt.perl.org/rt3/Ticket/Display.html?id=112126, Rich Haberman
notes that this package (version 0.11, present in both Perl 5.14.2 and
blead) implements subroutines called hash_unlocked() and
hashref_unlocked() but fails to document them in its SYNOPSIS or even to
include them in its @EXPORT_OK. The package does include two
subroutines, hash_locked() and hashref_locked(), in its @EXPORT_OK --
but fails to implement them!
I submitted a patch
(https://rt.perl.org/rt3/Ticket/Attachment/1109242/557222/0001-Add-subroutines-hash_locked-and-hashref_locked-to-Ha.patch)
to address this problem. The patch implemented hash_locked() and
hashref_locked(), modified the implementations of hash_unlocked() and
hashref_unlocked() appropriately -- or so I thought -- and brought the
package's @EXPORT_OK and SYNOPSIS and the list of subroutines tested by
can_ok() in t/Util.t into accordance with one another.
At that point, I wanted to see how well t/Util.t exercised Hash-Util's
functionality. Trying to do this while it was sitting in the Perl 5
source tree would have been cumbersome, so I created a CPAN-style
distribution for it which you can find here:
https://github.com/jkeenan/Hash-Util. I had to temporarily comment out
one unit test where %ENV was being locked as that impeded use of
Devel::Cover for coverage analysis
(http://www.nntp.perl.org/group/perl.qa/2012/04/msg13181.html).
At that point I felt I was in a good position to see how good the test
coverage was and, armed with that knowledge, write additional tests and
revise the package as needed. To my surprise, the additonal coverage
ratios were poor:
------------------------------------------- ------ ------
File stmt total
------------------------------------------- ------ ------
Util.xs 88.2 88.2
blib/lib/Hash/Util.pm 82.9 82.9
Total 83.6 83.6
------------------------------------------- ------ ------
4 subroutines -- lock_hashref_recurse(), lock_hash_recurse(),
unlock_hashref_recurse() and unlock_hash_recurse() -- were not tested at
all. But before I could write tests for them I had to write tests for
the subroutines I had just added.
First, I explicitly locked a hash, tested it with hash_locked() and
hashref_locked(), unlocked it and tested it with hash_unlocked() and
hashref_unlocked().
{
my %hash = (foo => 42, bar => 23);
lock_hash( %hash );
ok( hashref_locked( { %hash } ), 'hashref_locked' );
ok( hash_locked( %hash ), 'hash_locked' );
# snip
unlock_hash ( %hash );
ok( hashref_unlocked( { %hash } ), 'hashref_unlocked' );
ok( hash_unlocked( %hash ), 'hash_unlocked' );
# snip
}
My results DWIMmed:
ok 58 - hashref_locked
ok 59 - hash_locked
...
ok 63 - hashref_unlocked
ok 64 - hash_unlocked
I next created a hash which I did not lock. I would have expected that
when I called hash_locked() and hashref_locked on this hash, the return
values would have been Perl-false. Conversely, I would have expected
that once I locked that hash and then called hash_unlocked() and
hashref_unlocked on this hash, the return values would also have been
Perl-false.
my %hash = (foo => 42, bar => 23);
ok( ! hashref_locked( { %hash } ), 'hashref_locked negated' );
ok( ! hash_locked( %hash ), 'hash_locked negated' );
lock_hash( %hash );
ok( ! hashref_unlocked( { %hash } ), 'hashref_unlocked negated' );
ok( ! hash_unlocked( %hash ), 'hash_unlocked negated' );
But, to my surprise, all four of these tests FAILed and had to be
TODO-ed. In fact, when you take a hash and never lock it at all, the
hash*_locked() and hash*_unlocked() functions all return true at the
same time:
{
my %hash = (foo => 42, bar => 23);
ok( hashref_locked( { %hash } ), 'hashref_locked' );
ok( hash_locked( %hash ), 'hash_locked' );
ok( hashref_unlocked( { %hash } ), 'hashref_unlocked' );
ok( hash_unlocked( %hash ), 'hash_unlocked' );
}
Now it could be that my implementation of these functions is simply
incorrect. Here they are:
sub hashref_locked {
my $hash=shift;
Internals::SvREADONLY($hash) ? return 0 : return 1;
}
sub hash_locked(\%) { hashref_locked(@_) }
sub hashref_unlocked {
my $hash=shift;
(! Internals::SvREADONLY($hash)) ? return 1 : return 0;
}
sub hash_unlocked(\%) { hashref_unlocked(@_) }
As noted above, only the *_unlocked functions are implemented in version
0.11:
sub hashref_unlocked {
my $hash=shift;
return Internals::SvREADONLY($hash)
}
sub hash_unlocked(\%) { hashref_unlocked(@_) }
I don't think my implementations differ too much from that, but then
again, the *_unlocked() functions are not currently being tested by
t/Util.t in blead.
I'm increasingly suspicious of the use of Internals::SvREADONLY in
version 0.11's definition of hashref_unlocked(). And therefore I'm
suspicious of my own use of Internals::SvREADONLY in my version of the
package as well. I don't really know what Internals::SvREADONLY is or
does. The code for SvREADONLY in universal.c is undocumented except for
these comments:
XS(XS_Internals_SvREADONLY) /* This is dangerous stuff. */
{
dVAR;
dXSARGS;
SV * const svz = ST(0);
SV * sv;
PERL_UNUSED_ARG(cv);
/* [perl #77776] - called as &foo() not foo() */
if (!SvROK(svz))
croak_xs_usage(cv, "SCALAR[, ON]");
sv = SvRV(svz);
if (items == 1) {
if (SvREADONLY(sv) && !SvIsCOW(sv))
XSRETURN_YES;
else
XSRETURN_NO;
}
else if (items == 2) {
if (SvTRUE(ST(1))) {
if (SvIsCOW(sv)) sv_force_normal(sv);
SvREADONLY_on(sv);
XSRETURN_YES;
}
else {
/* I hope you really know what you are doing. */
if (!SvIsCOW(sv)) SvREADONLY_off(sv);
XSRETURN_NO;
}
}
XSRETURN_UNDEF; /* Can't happen. */
}
To sum up:
My attempts to correct evident deficiencies in Hash-Util are stumbling.
We ought to be able to write functions which accurately report whether
a given hash or hashref is currently locked or not, but the results I'm
getting so far don't make sense. My hunch is that the use of the
undocumented Internals::SvREADONLY is implicated in the problem.
Comments?
Thank you very much.
Jim Keenan
---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126
|
0001-Add-subroutines-hash_locked-and-hashref_locked-to-Ha.patch
