Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Perl: porters

[perl #112126] Hash::Util export bug

 

 

Perl porters RSS feed   Index | Next | Previous | View Threaded


perlbug-followup at perl

Apr 21, 2012, 6:07 AM

Post #1 of 20 (1116 views)
Permalink
[perl #112126] Hash::Util export bug

On Thu Mar 29 20:25:21 2012, rich.a.haberman [at] gmail wrote:
>
> -----------------------------------------------------------------
>
> The docs for Hash::Util state to use "hash_unlocked" to test that a
> %hash is
> locked.
>
> hash_unlocked
>
> hash_unlocked(%hash) and print "Hash is unlocked!\n";
>
> Returns true if the hash and its keys are unlocked.
>
>
> But the docs for Hash::Util show that only "hash_locked" can be
> imported
>
> use Hash::Util qw(
> hash_seed all_keys
> lock_keys unlock_keys
> lock_value unlock_value
> lock_hash unlock_hash
> lock_keys_plus hash_locked
> hidden_keys legal_keys
> );
>
> and the Synopsis states:
>
> # Ways to inspect the properties of a restricted hash
> my @legal = legal_keys(%hash);
> my @hidden = hidden_keys(%hash);
> my $ref = all_keys(%hash,@keys,@hidden);
> my $is_locked = hash_locked(%hash);
>
>
> In Hash::Util.pm there is no "hash_locked", there is only:
>
> sub hash_unlocked(\%) { hashref_unlocked(@_) }
>
>
> and the @EXPORT_OK in Hash::Util.pm is:
>
> our @EXPORT_OK = qw(
> fieldhash fieldhashes
>
> all_keys
> lock_keys unlock_keys
> lock_value unlock_value
> lock_hash unlock_hash
> lock_keys_plus hash_locked
> hidden_keys legal_keys
>
> lock_ref_keys unlock_ref_keys
> lock_ref_value unlock_ref_value
> lock_hashref unlock_hashref
> lock_ref_keys_plus hashref_locked
> hidden_ref_keys legal_ref_keys
>
> hash_seed hv_store
>
> );
>
> so there is no way to import "hash_unlocked"
>
>
> Fix: change "hash_locked" to "hash_unlocked" in @EXPORT_OK of
> Hash::Util.pm,
> correct pod references to nonexistent "hash_locked"
>
>

This appears to be a valid complaint; the line of approach to fixing it
also appears to be correct.

cc-ing module's authors listed in documentation for comment.

---
via perlbug: queue: perl5 status: new
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


demerphq at gmail

Apr 22, 2012, 3:54 AM

Post #2 of 20 (1106 views)
Permalink
Re: [perl #112126] Hash::Util export bug [In reply to]

On 21 April 2012 15:07, James E Keenan via RT <perlbug-followup [at] perl> wrote:
> On Thu Mar 29 20:25:21 2012, rich.a.haberman [at] gmail wrote:
>>
>> -----------------------------------------------------------------
>>
>> The docs for Hash::Util state to use "hash_unlocked" to test that a
>> %hash is
>> locked.
>>
>> hash_unlocked
>>
>> hash_unlocked(%hash) and print "Hash is unlocked!\n";
>>
>> Returns true if the hash and its keys are unlocked.
>>
>>
>> But the docs for Hash::Util show that only "hash_locked" can be
>> imported
>>
>> use Hash::Util qw(
>> hash_seed all_keys
>> lock_keys unlock_keys
>> lock_value unlock_value
>> lock_hash unlock_hash
>> lock_keys_plus hash_locked
>> hidden_keys legal_keys
>> );
>>
>> and the Synopsis states:
>>
>> # Ways to inspect the properties of a restricted hash
>> my @legal = legal_keys(%hash);
>> my @hidden = hidden_keys(%hash);
>> my $ref = all_keys(%hash,@keys,@hidden);
>> my $is_locked = hash_locked(%hash);
>>
>>
>> In Hash::Util.pm there is no "hash_locked", there is only:
>>
>> sub hash_unlocked(\%) { hashref_unlocked(@_) }
>>
>>
>> and the @EXPORT_OK in Hash::Util.pm is:
>>
>> our @EXPORT_OK = qw(
>> fieldhash fieldhashes
>>
>> all_keys
>> lock_keys unlock_keys
>> lock_value unlock_value
>> lock_hash unlock_hash
>> lock_keys_plus hash_locked
>> hidden_keys legal_keys
>>
>> lock_ref_keys unlock_ref_keys
>> lock_ref_value unlock_ref_value
>> lock_hashref unlock_hashref
>> lock_ref_keys_plus hashref_locked
>> hidden_ref_keys legal_ref_keys
>>
>> hash_seed hv_store
>>
>> );
>>
>> so there is no way to import "hash_unlocked"
>>
>>
>> Fix: change "hash_locked" to "hash_unlocked" in @EXPORT_OK of
>> Hash::Util.pm,
>> correct pod references to nonexistent "hash_locked"
>>
>>
>
> This appears to be a valid complaint; the line of approach to fixing it
> also appears to be correct.
>
> cc-ing module's authors listed in documentation for comment.

And/or add hash_locked() as a negation of hash_unlocked()

Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"


perlbug-followup at perl

Apr 22, 2012, 6:54 PM

Post #3 of 20 (1109 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

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.

Thank you very much.
Jim Keenan

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126
Attachments: 0001-Add-subroutines-hash_locked-and-hashref_locked-to-Ha.patch (7.60 KB)


perlbug-followup at perl

Apr 24, 2012, 6:03 PM

Post #4 of 20 (1105 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

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.

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 25, 2012, 6:13 PM

Post #5 of 20 (1105 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Tue Apr 24 18:03:20 2012, sprout wrote:
>
> I’ve pushed these to the sprout/misc-post-5.16 branch for now.
>

Note: I suspect that since I added some functions to the module, I
should have bumped $Hash::Util::VERSION to 0.12 from 0.11.

Would changing it within the module be sufficient? I got a message like
this when testing locally after setting $VERSION to 0.12:

#####

Hash::Util object version 0.11 does not match $Hash::Util::VERSION 0.12
at /usr/local/lib/perl5/5.14.2/darwin-2level/DynaLoader.pm line 217.

#####

Thank you very mcuh.
Jim Keenan





---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 25, 2012, 8:33 PM

Post #6 of 20 (1097 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Wed Apr 25 18:13:18 2012, jkeenan wrote:
> On Tue Apr 24 18:03:20 2012, sprout wrote:
> >
> > I’ve pushed these to the sprout/misc-post-5.16 branch for now.
> >
>
> Note: I suspect that since I added some functions to the module, I
> should have bumped $Hash::Util::VERSION to 0.12 from 0.11.
>
> Would changing it within the module be sufficient? I got a message like
> this when testing locally after setting $VERSION to 0.12:
>
> #####
>
> Hash::Util object version 0.11 does not match $Hash::Util::VERSION 0.12
> at /usr/local/lib/perl5/5.14.2/darwin-2level/DynaLoader.pm line 217.
>
> #####

Usually leaving it to the committers is sufficient. If the patch is not
applied when you expect, then there will be a conflict.

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


demerphq at gmail

Apr 26, 2012, 12:55 AM

Post #7 of 20 (1096 views)
Permalink
Re: [perl #112126] Hash::Util export bug [In reply to]

On 26 April 2012 03:13, James E Keenan via RT <perlbug-followup [at] perl> wrote:
> On Tue Apr 24 18:03:20 2012, sprout wrote:
>>
>> Ive pushed these to the sprout/misc-post-5.16 branch for now.
>>
>
> Note: I suspect that since I added some functions to the module, I
> should have bumped $Hash::Util::VERSION to 0.12 from 0.11.
>
> Would changing it within the module be sufficient? I got a message like
> this when testing locally after setting $VERSION to 0.12:
>
> #####
>
> Hash::Util object version 0.11 does not match $Hash::Util::VERSION 0.12
> at /usr/local/lib/perl5/5.14.2/darwin-2level/DynaLoader.pm line 217.
>
> #####
>

Try doing a full clean on the code then re-making. I think there might
be an issue with it compiling in version info in a way that make
doesnt see.

Yves


--
perl -Mre=debug -e "/just|another|perl|hacker/"


perlbug-followup at perl

Apr 26, 2012, 9:49 AM

Post #8 of 20 (1100 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Thu Apr 26 00:55:53 2012, demerphq wrote:
> On 26 April 2012 03:13, James E Keenan via RT <perlbug-
> followup [at] perl> wrote:
> > On Tue Apr 24 18:03:20 2012, sprout wrote:
> >>
> >> I’ve pushed these to the sprout/misc-post-5.16 branch for now.
> >>
> >
> > Note:  I suspect that since I added some functions to the module, I
> > should have bumped $Hash::Util::VERSION to 0.12 from 0.11.
> >
> > Would changing it within the module be sufficient?  I got a message
> like
> > this when testing locally after setting $VERSION to 0.12:
> >
> > #####
> >
> > Hash::Util object version 0.11 does not match $Hash::Util::VERSION
> 0.12
> > at /usr/local/lib/perl5/5.14.2/darwin-2level/DynaLoader.pm line 217.
> >
> > #####
> >
>
> Try doing a full clean on the code then re-making. I think there might
> be an issue with it compiling in version info in a way that make
> doesnt see.

To avoid recompiling everything, sometimes I just delete the Makefile
(ext/Hash-Util/Makefile) and re-run make. It doesn’t always work, but
it seems to most of the time.

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 28, 2012, 5:41 PM

Post #9 of 20 (1099 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

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


perlbug-followup at perl

Apr 28, 2012, 5:59 PM

Post #10 of 20 (1105 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sat Apr 28 17:41:43 2012, jkeenan wrote:
> 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;
> }

I think I responded a little too hastily earlier.

You are returning the same thing in both hashref_locked and
hashref_unlocked.

In the first, you have ‘return 0’ when SvREADONLY is true.

In the second, you have ‘return 0’ when !SvREADONLY is false.

Your double negatives make my head hurt. I find it much easier to read
like this (with hashref_locked corrected):

sub hashref_locked {
my $hash=shift;
!Internals::SvREADONLY($hash);
}

sub hashref_unlocked {
my $hash=shift;
Internals::SvREADONLY($hash);
}

which makes the difference obvious.

>
> 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)
> }

That’s wrong, because the SvREADONLY flag means locked.

>
> I don't really know what Internals::SvREADONLY is or
> does.

SvREADONLY will return true for any read-only scalar, like $].

SvREADONLY on a hash means it is locked.

Internally, at the C level, SvREADONLY also means copy-on-write, which
is why the code you cite below uses SvIsCOW to distinguish the cases
(i.e., Internals::SvREADONLY doesn’t strictly flip the flag on and off,
as that is not what Hash::Util needs). sv_force_normal stops a COW
scalar from being such.

Knowing that, I find the code self-documenting, but I’m getting too
close to the core these days to know what would be obvious to other
people. :-)

> 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. */
> }


--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


doy at tozt

Apr 28, 2012, 7:54 PM

Post #11 of 20 (1102 views)
Permalink
Re: [perl #112126] Hash::Util export bug [In reply to]

On Sat, Apr 28, 2012 at 05:59:57PM -0700, Father Chrysostomos via RT wrote:
> On Sat Apr 28 17:41:43 2012, jkeenan wrote:
> > 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;
> > }
>
> I think I responded a little too hastily earlier.
>
> You are returning the same thing in both hashref_locked and
> hashref_unlocked.
>
> In the first, you have ‘return 0’ when SvREADONLY is true.
>
> In the second, you have ‘return 0’ when !SvREADONLY is false.
>
> Your double negatives make my head hurt. I find it much easier to read
> like this (with hashref_locked corrected):
>
> sub hashref_locked {
> my $hash=shift;
> !Internals::SvREADONLY($hash);
> }
>
> sub hashref_unlocked {
> my $hash=shift;
> Internals::SvREADONLY($hash);
> }
>
> which makes the difference obvious.
>
> >
> > 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)
> > }
>
> That’s wrong, because the SvREADONLY flag means locked.
>
> >
> > I don't really know what Internals::SvREADONLY is or
> > does.
>
> SvREADONLY will return true for any read-only scalar, like $].
>
> SvREADONLY on a hash means it is locked.
>
> Internally, at the C level, SvREADONLY also means copy-on-write, which
> is why the code you cite below uses SvIsCOW to distinguish the cases
> (i.e., Internals::SvREADONLY doesn’t strictly flip the flag on and off,
> as that is not what Hash::Util needs). sv_force_normal stops a COW
> scalar from being such.
>
> Knowing that, I find the code self-documenting, but I’m getting too
> close to the core these days to know what would be obvious to other
> people. :-)

Which makes me wonder why SvREADONLY is exposed to perl-space at all.
It's not like it even keeps Hash::Util from needing XS. Wouldn't it make
things a lot easier to follow if Internals::SvREADONLY was removed in
favor of just adding a few bits to ext/Hash-Util/Util.xs?

-doy


perlbug-followup at perl

Apr 28, 2012, 7:59 PM

Post #12 of 20 (1096 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sat Apr 28 17:59:57 2012, sprout wrote:
>
> Your double negatives make my head hurt. I find it much easier to read
> like this (with hashref_locked corrected):
>
> sub hashref_locked {
> my $hash=shift;
> !Internals::SvREADONLY($hash);
> }
>
> sub hashref_unlocked {
> my $hash=shift;
> Internals::SvREADONLY($hash);
> }
>
> which makes the difference obvious.
>


Let's say I try that. (See:
https://github.com/jkeenan/Hash-Util/tree/112126/sprout)

##########

diff --git a/lib/Hash/Util.pm b/lib/Hash/Util.pm
index bfe101b..d830b0d 100644
--- a/lib/Hash/Util.pm
+++ b/lib/Hash/Util.pm
@@ -372,7 +372,7 @@ Returns true if the hash and its keys are locked.

sub hashref_locked {
my $hash=shift;
- Internals::SvREADONLY($hash) ? return 0 : return 1;
+ !Internals::SvREADONLY($hash);
}

sub hash_locked(\%) { hashref_locked(@_) }
@@ -390,7 +390,7 @@ Returns true if the hash and its keys are unlocked.

sub hashref_unlocked {
my $hash=shift;
- (! Internals::SvREADONLY($hash)) ? return 1 : return 0;
+ Internals::SvREADONLY($hash);
}

sub hash_unlocked(\%) { hashref_unlocked(@_) }

##########

Unfortunately, two tests that were passing now fail while two tests that
were failing (and TODOed) now pass:

ok 58 - hashref_locked
ok 59 - hash_locked
ok 60 - Was locked %hash
ok 61 - Was locked $hash{foo}
ok 62 - Was locked $hash{bar}
not ok 63 - hashref_unlocked
not ok 64 - hash_unlocked
ok 65 - Was unlocked %hash
ok 66 - Was unlocked $hash{foo}
ok 67 - Was unlocked $hash{bar}
not ok 68 - hashref_locked negated # TODO negated hash(ref)_(un)locked
not yet working

# Failed (TODO) test 'hashref_locked negated'
# at t/Util.t line 174.
not ok 69 - hash_locked negated # TODO negated hash(ref)_(un)locked not
yet working

# Failed (TODO) test 'hash_locked negated'
# at t/Util.t line 175.
ok 70 - hashref_unlocked negated # TODO negated hash(ref)_(un)locked not
yet working
ok 71 - hash_unlocked negated # TODO negated hash(ref)_(un)locked not
yet working


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 28, 2012, 9:15 PM

Post #13 of 20 (1097 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sat Apr 28 19:55:04 2012, doy [at] tozt wrote:
> Which makes me wonder why SvREADONLY is exposed to perl-space at all.
> It's not like it even keeps Hash::Util from needing XS. Wouldn't it make
> things a lot easier to follow if Internals::SvREADONLY was removed in
> favor of just adding a few bits to ext/Hash-Util/Util.xs?

I would favour that for the sake of speed. Is Internals::SvREADONLY
(ab)used anywhere else in the core, apart from tests?

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 28, 2012, 9:17 PM

Post #14 of 20 (1101 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sat Apr 28 19:59:55 2012, jkeenan wrote:
> On Sat Apr 28 17:59:57 2012, sprout wrote:
> >
> > Your double negatives make my head hurt.

To the point of getting things backward myself!

> > I find it much easier to read
> > like this (with hashref_locked corrected):
> >
> > sub hashref_locked {
> > my $hash=shift;
> > !Internals::SvREADONLY($hash);
> > }
> >
> > sub hashref_unlocked {
> > my $hash=shift;
> > Internals::SvREADONLY($hash);
> > }
> >
> > which makes the difference obvious.
> >
>
>
> Let's say I try that. (See:
> https://github.com/jkeenan/Hash-Util/tree/112126/sprout)
>
> ##########
>
> diff --git a/lib/Hash/Util.pm b/lib/Hash/Util.pm
> index bfe101b..d830b0d 100644
> --- a/lib/Hash/Util.pm
> +++ b/lib/Hash/Util.pm
> @@ -372,7 +372,7 @@ Returns true if the hash and its keys are locked.
>
> sub hashref_locked {
> my $hash=shift;
> - Internals::SvREADONLY($hash) ? return 0 : return 1;
> + !Internals::SvREADONLY($hash);
> }
>
> sub hash_locked(\%) { hashref_locked(@_) }
> @@ -390,7 +390,7 @@ Returns true if the hash and its keys are unlocked.
>
> sub hashref_unlocked {
> my $hash=shift;
> - (! Internals::SvREADONLY($hash)) ? return 1 : return 0;
> + Internals::SvREADONLY($hash);
> }
>
> sub hash_unlocked(\%) { hashref_unlocked(@_) }
>
> ##########
>
> Unfortunately, two tests that were passing now fail while two tests that
> were failing (and TODOed) now pass:

Well, this is a good lesson in humility. I got them reversed, too.

Now move the exclamation mark from hashref_locked to hashref_unlocked. :-)

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 29, 2012, 5:00 AM

Post #15 of 20 (1097 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sat Apr 28 21:17:11 2012, sprout wrote:
> On Sat Apr 28 19:59:55 2012, jkeenan wrote:

> Well, this is a good lesson in humility. I got them reversed, too.
>
> Now move the exclamation mark from hashref_locked to hashref_unlocked. :-)
>

Unfortunately, that simply switches around the tests that FAIL or
unexpectedly PASS.

##########
# https://github.com/jkeenan/Hash-Util/tree/112126/sprout

$ git show e3599b5
commit e3599b5083b93ddb1f18de6da140292290913414
Author: jkeenan <jkeenan [at] cpan>
Date: Sun Apr 29 07:49:51 2012 -0400

Father C said switch the \!

diff --git a/lib/Hash/Util.pm b/lib/Hash/Util.pm
index d830b0d..bb8a981 100644
--- a/lib/Hash/Util.pm
+++ b/lib/Hash/Util.pm
@@ -372,7 +372,7 @@ Returns true if the hash and its keys are locked.

sub hashref_locked {
my $hash=shift;
- !Internals::SvREADONLY($hash);
+ Internals::SvREADONLY($hash);
}

sub hash_locked(\%) { hashref_locked(@_) }
@@ -390,7 +390,7 @@ Returns true if the hash and its keys are unlocked.

sub hashref_unlocked {
my $hash=shift;
- Internals::SvREADONLY($hash);
+ !Internals::SvREADONLY($hash);
}

sub hash_unlocked(\%) { hashref_unlocked(@_) }

##########

PERL_DL_NONLAZY=1 /usr/local/bin/perl "-MExtUtils::Command::MM" "-e"
"test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/Util.t .. 1/246
# Failed test 'hashref_locked'
# at t/Util.t line 155.

# Failed test 'hash_locked'
# at t/Util.t line 156.
# Looks like you failed 2 tests of 246.
t/Util.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/246 subtests
(2 TODO tests unexpectedly succeeded)

Test Summary Report
-------------------
t/Util.t (Wstat: 512 Tests: 246 Failed: 2)
Failed tests: 58-59
TODO passed: 68-69
Non-zero exit status: 2
Files=1, Tests=246, 1 wallclock secs ( 0.22 usr 0.06 sys + 0.34 cusr
0.07 csys = 0.69 CPU)
Result: FAIL
Failed 1/1 test programs. 2/246 subtests failed.
make: *** [test_dynamic] Error 2

Thank you very much.
Jim Keenan


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 29, 2012, 5:21 AM

Post #16 of 20 (1095 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sat Apr 28 21:15:41 2012, sprout wrote:
> On Sat Apr 28 19:55:04 2012, doy [at] tozt wrote:
> > Which makes me wonder why SvREADONLY is exposed to perl-space at all.
> > It's not like it even keeps Hash::Util from needing XS. Wouldn't it make
> > things a lot easier to follow if Internals::SvREADONLY was removed in
> > favor of just adding a few bits to ext/Hash-Util/Util.xs?
>
> I would favour that for the sake of speed. Is Internals::SvREADONLY
> (ab)used anywhere else in the core, apart from tests?
>

Jesse: Patches to the XS welcome! After we solve these immediate
problems, I would certainly like to eliminate Internals::SvREADONLY from
this t/Util.t.

Father C: The attached list is a start. It is the output of:

ack -l SvREADONLY lib/ ext/ dist/ cpan/ >
~/learn/perl/p5p/SvREADONLY_usages_in_distros.txt

Then, manually edit the result to eliminate generated .c files.

Thank you very much.
Jim Keenan


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126
Attachments: SvREADONLY_usages_in_distros.txt (0.79 KB)


perlbug-followup at perl

Apr 29, 2012, 10:52 AM

Post #17 of 20 (1104 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sun Apr 29 05:21:08 2012, jkeenan wrote:
> Father C: The attached list is a start. It is the output of:
>
> ack -l SvREADONLY lib/ ext/ dist/ cpan/ >
> ~/learn/perl/p5p/SvREADONLY_usages_in_distros.txt
>
> Then, manually edit the result to eliminate generated .c files.

I see it begins with:

> lib/constant.pm

which I had forgotten about. constant.pm needs Internals::SvREADONLY,
so there’s little chance of eliminating it, which was what I was hoping for.

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126


perlbug-followup at perl

Apr 29, 2012, 10:53 AM

Post #18 of 20 (1098 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sun Apr 29 05:00:19 2012, jkeenan wrote:
> On Sat Apr 28 21:17:11 2012, sprout wrote:
> > On Sat Apr 28 19:59:55 2012, jkeenan wrote:
>
> > Well, this is a good lesson in humility. I got them reversed, too.
> >
> > Now move the exclamation mark from hashref_locked to
hashref_unlocked. :-)
> >
>
> Unfortunately, that simply switches around the tests that FAIL or
> unexpectedly PASS.
>
> ##########
> # https://github.com/jkeenan/Hash-Util/tree/112126/sprout
>
> $ git show e3599b5
> commit e3599b5083b93ddb1f18de6da140292290913414
> Author: jkeenan <jkeenan [at] cpan>
> Date: Sun Apr 29 07:49:51 2012 -0400
>
> Father C said switch the \!
>
> diff --git a/lib/Hash/Util.pm b/lib/Hash/Util.pm
> index d830b0d..bb8a981 100644
> --- a/lib/Hash/Util.pm
> +++ b/lib/Hash/Util.pm
> @@ -372,7 +372,7 @@ Returns true if the hash and its keys are locked.
>
> sub hashref_locked {
> my $hash=shift;
> - !Internals::SvREADONLY($hash);
> + Internals::SvREADONLY($hash);
> }
>
> sub hash_locked(\%) { hashref_locked(@_) }
> @@ -390,7 +390,7 @@ Returns true if the hash and its keys are unlocked.
>
> sub hashref_unlocked {
> my $hash=shift;
> - Internals::SvREADONLY($hash);
> + !Internals::SvREADONLY($hash);
> }
>
> sub hash_unlocked(\%) { hashref_unlocked(@_) }
>
> ##########
>
> PERL_DL_NONLAZY=1 /usr/local/bin/perl "-MExtUtils::Command::MM" "-e"
> "test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
> t/Util.t .. 1/246
> # Failed test 'hashref_locked'
> # at t/Util.t line 155.
>
> # Failed test 'hash_locked'
> # at t/Util.t line 156.
> # Looks like you failed 2 tests of 246.
> t/Util.t .. Dubious, test returned 2 (wstat 512, 0x200)
> Failed 2/246 subtests
> (2 TODO tests unexpectedly succeeded)
>
> Test Summary Report
> -------------------
> t/Util.t (Wstat: 512 Tests: 246 Failed: 2)
> Failed tests: 58-59
> TODO passed: 68-69
> Non-zero exit status: 2
> Files=1, Tests=246, 1 wallclock secs ( 0.22 usr 0.06 sys + 0.34 cusr
> 0.07 csys = 0.69 CPU)
> Result: FAIL
> Failed 1/1 test programs. 2/246 subtests failed.
> make: *** [test_dynamic] Error 2

Instead of giving another armchair response, I thought I’d better
actually run some tests. :-)

You’ll notice that lock_ref_keys does Internals::SvREADONLY(%$hash),
while the two problematic functions do Internals::SvREADONLY($hash).
*That’s* the problem. It was checking to see whether the $hash scalar
was read-only.

See the attached diff, which is against the 112126/sprout branch in your
repository.

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126
Attachments: open_ueIaObSa.txt (1.34 KB)


perlbug-followup at perl

Apr 29, 2012, 6:30 PM

Post #19 of 20 (1101 views)
Permalink
[perl #112126] Hash::Util export bug [In reply to]

On Sun Apr 29 10:53:12 2012, sprout wrote:
>
> Instead of giving another armchair response, I thought I’d better
> actually run some tests. :-)
>
> You’ll notice that lock_ref_keys does Internals::SvREADONLY(%$hash),
> while the two problematic functions do Internals::SvREADONLY($hash).
> *That’s* the problem. It was checking to see whether the $hash scalar
> was read-only.
>

Ah! The power of multiple application of multiple eyeballs!

> See the attached diff, which is against the 112126/sprout branch in your
> repository.
>

Yes, that resolved the problem. I applied your patch to that branch,
tested it, merged it into that repository's master, tested again, then
manually applied the patches to my local branch of blead.

I believe that if you apply the patch attached,
0001-Document-hashref_locked-and-hashref_unlocked-.-Add-t.patch, in
addition to the patch I submitted on Apr 22, you should get a better
ext/Hash-Util/lib/Hash/Util.pm as well as an ext/Hash-Util/t/Util.t that
provides 100% statement coverge of that package.

Thank you very much.
Jim Keenan



---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112126
Attachments: 0001-Document-hashref_locked-and-hashref_unlocked-.-Add-t.patch (8.14 KB)


nick at ccl4

May 8, 2012, 4:27 AM

Post #20 of 20 (1072 views)
Permalink
Re: [perl #112126] Hash::Util export bug [In reply to]

On Sat, Apr 28, 2012 at 09:54:31PM -0500, Jesse Luehrs wrote:

> Which makes me wonder why SvREADONLY is exposed to perl-space at all.
> It's not like it even keeps Hash::Util from needing XS. Wouldn't it make
> things a lot easier to follow if Internals::SvREADONLY was removed in
> favor of just adding a few bits to ext/Hash-Util/Util.xs?

I think Hash::Util would be easier to read if it better encapsulated the
unfortunate fact that it overloads the use of SVf_READONLY to mean a locked
hash, by putting the relevant necessary C-level manipulation into one well-
named place in its XS code.

Yes, constant.pm is (currently) using Internals::SvREADONLY(), but for the
"conventional" purpose (a read only scalar value). That's unfortunate. I
wonder how to fix that. I'm even wondering if it's actually necessary for
constant.pm to do that, or if it's just a "nice to have". (In that, nothing
relies on enforcement of not being able to manipulate references in the
symbol table. It's not like it can't be worked round)

Nicholas Clark

Perl porters RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.