Skip to content
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

Hash::Util export bug #12025

Closed
p5pRT opened this issue Mar 30, 2012 · 28 comments
Closed

Hash::Util export bug #12025

p5pRT opened this issue Mar 30, 2012 · 28 comments

Comments

@p5pRT
Copy link

p5pRT commented Mar 30, 2012

Migrated from rt.perl.org#112126 (status was 'resolved')

Searchable as RT112126$

@p5pRT
Copy link
Author

p5pRT commented Mar 30, 2012

From rich.a.haberman@gmail.com

Created by rich.a.haberman@gmail.com

This is a bug report for perl from rich.a.haberman@​gmail.com,
generated with the help of perlbug 1.39 running under perl 5.14.2.

-----------------------------------------------------------------

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"

Perl Info

Flags:
    category=library
    severity=low
    module=Hash::Util

Site configuration information for perl 5.14.2:

Configured by gecko at Fri Oct  7 15:46:51 2011.

Summary of my perl5 (revision 5 version 14 subversion 2) configuration:

  Platform:
    osname=MSWin32, osvers=5.2, archname=MSWin32-x86-multi-thread
    uname=''
    config_args='undef'
    hint=recommended, useposix=true, d_sigaction=undef
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='C:/mingw/bin/gcc.exe', ccflags
='-DNDEBUG -DWIN32 -D_CONSOLE -DNO_STRICT -DPERL_TEXTMODE_SCRIPTS -DUSE_SITECUSTOMIZE
 -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T
 -DHASATTRIBUTE -fno-strict-aliasing -mms-bitfields',
    optimize='-O2',
    cppflags='-DWIN32'
    ccversion='', gccversion='4.6.2', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='C:\mingw\bin\g++.exe', ldflags ='-L"C:\Perl\lib\CORE"'
    libpth=\lib
    libs=-lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32
 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32
 -lodbccp32 -lcomctl32 -lmsvcrt
    perllibs=-lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32
 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32
 -lodbccp32 -lcomctl32 -lmsvcrt
    libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl514.lib
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-mdll -L"C:\Perl\lib\CORE"'

Locally applied patches:
    ACTIVEPERL_LOCAL_PATCHES_ENTRY


@INC for perl 5.14.2:
    C:/Perl/site/lib
    C:/Perl/lib
    .


Environment for perl 5.14.2:
    HOME (unset)
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=C:\WINDOWS;C:\WINDOWS\system32;C:\Perl\bin;C:\php;C:\python;C:\bin;C:\mingw\bin;C:\msys\1.0\bin;C:\swigwin-2.0.4;C:\ghc\ghc-7.0.3\bin;C:\ImageMagick;C:\Perl\site\bin;C:\Perl-5.12\perl\bin;C:\Perl-5.12\perl\site\bin;C:\Program
Files\ActiveState Komodo Edit 5\;C:\Program Files\ATI
Technologies\ATI.ACE\Core-Static;C:\Program Files\Git\cmd;C:\Program
Files\Haskell\bin;C:\Program Files\Java\jdk1.6.0_20\bin;C:\Program
Files\Microsoft SQL Server\100\DTS\Binn\;C:\Program Files\Microsoft SQL
Server\100\Tools\Binn\;C:\Program Files\pcc\bin;C:\Program Files\Smart
Projects\IsoBuster;C:\Python\Lib\site-packages\PyQt4\bin;C:\python\Scripts;C:\Python31;C:\Ruby193\bin;C:\zeromq\lib;C:\WINDOWS\System32\Wbem;C:\WINDOWS\system32\WindowsPowerShell\v1.0;C:\Parrot-3.10.0\bin;C:\Program
Files\SlikSvn\bin;C:\Program Files\TortoiseGit\bin;%APPDATA%\Python\Scripts
    PERL_BADLANG (unset)
    SHELL (unset)


@p5pRT
Copy link
Author

p5pRT commented Apr 21, 2012

From @jkeenan

On Thu Mar 29 20​:25​:21 2012, rich.a.haberman@​gmail.com 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.

@p5pRT
Copy link
Author

p5pRT commented Apr 21, 2012

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2012

From @demerphq

On 21 April 2012 15​:07, James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

On Thu Mar 29 20​:25​:21 2012, rich.a.haberman@​gmail.com 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/"

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2012

From @jkeenan

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

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2012

From @jkeenan

0001-Add-subroutines-hash_locked-and-hashref_locked-to-Ha.patch
From daa8ff31e8b76322f40147e217e45ac0cd162a4a Mon Sep 17 00:00:00 2001
From: jkeenan <jkeenan@cpan.org>
Date: Sun, 22 Apr 2012 20:59:33 -0400
Subject: [PATCH] Add subroutines hash_locked() and hashref_locked() to Hash::Util.

Make @EXPORT_OK, synopsis, and list of functions tested with
can_ok() consistent with one another.  Rationalize the way
functions are grouped within @EXPORT_OK and the other locations.
Add tests for hash_locked(), hashref_locked(), hash_unlocked() and
hashref_unlocked().  Add descriptions for several unit tests which
lacked them.

For RT #112126.
---
 ext/Hash-Util/lib/Hash/Util.pm |   39 +++++++++++++++++++++++++++++++++------
 ext/Hash-Util/t/Util.t         |   34 +++++++++++++++++++---------------
 2 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/ext/Hash-Util/lib/Hash/Util.pm b/ext/Hash-Util/lib/Hash/Util.pm
index 8555821..dd2bb33 100644
--- a/ext/Hash-Util/lib/Hash/Util.pm
+++ b/ext/Hash-Util/lib/Hash/Util.pm
@@ -17,17 +17,18 @@ our @EXPORT_OK  = qw(
                      lock_keys unlock_keys
                      lock_value unlock_value
                      lock_hash unlock_hash
-                     lock_keys_plus hash_locked
+                     lock_keys_plus
+                     hash_locked hash_unlocked
+                     hashref_locked hashref_unlocked
                      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
+                     lock_ref_keys_plus
                      hidden_ref_keys legal_ref_keys
 
                      hash_seed hv_store
-
                     );
 our $VERSION = '0.11';
 require XSLoader;
@@ -53,12 +54,24 @@ Hash::Util - A selection of general-utility hash subroutines
   # Restricted hashes
 
   use Hash::Util qw(
-                     hash_seed all_keys
+                     fieldhash fieldhashes
+
+                     all_keys
                      lock_keys unlock_keys
                      lock_value unlock_value
                      lock_hash unlock_hash
-                     lock_keys_plus hash_locked
+                     lock_keys_plus
+                     hash_locked hash_unlocked
+                     hashref_locked hashref_unlocked
                      hidden_keys legal_keys
+
+                     lock_ref_keys unlock_ref_keys
+                     lock_ref_value unlock_ref_value
+                     lock_hashref unlock_hashref
+                     lock_ref_keys_plus
+                     hidden_ref_keys legal_ref_keys
+
+                     hash_seed hv_store
                    );
 
   %hash = (foo => 42, bar => 23);
@@ -346,6 +359,20 @@ sub unlock_hashref_recurse {
 sub   lock_hash_recurse (\%) {   lock_hashref_recurse(@_) }
 sub unlock_hash_recurse (\%) { unlock_hashref_recurse(@_) }
 
+=item B<hash_locked>
+
+  hash_locked(%hash) and print "Hash is locked!\n";
+
+Returns true if the hash and its keys are locked.
+
+=cut
+
+sub hashref_locked {
+    my $hash=shift;
+    Internals::SvREADONLY($hash) ? return 0 : return 1;
+}
+
+sub hash_locked(\%) { hashref_locked(@_) }
 
 =item B<hash_unlocked>
 
@@ -357,7 +384,7 @@ Returns true if the hash and its keys are unlocked.
 
 sub hashref_unlocked {
     my $hash=shift;
-    return Internals::SvREADONLY($hash)
+    (! Internals::SvREADONLY($hash)) ? return 1 : return 0;
 }
 
 sub hash_unlocked(\%) { hashref_unlocked(@_) }
diff --git a/ext/Hash-Util/t/Util.t b/ext/Hash-Util/t/Util.t
index 74d823d..fa0f66c 100644
--- a/ext/Hash-Util/t/Util.t
+++ b/ext/Hash-Util/t/Util.t
@@ -16,22 +16,26 @@ use Test::More;
 my @Exported_Funcs;
 BEGIN {
     @Exported_Funcs = qw(
-                     hash_seed all_keys
+                     fieldhash fieldhashes
+
+                     all_keys
                      lock_keys unlock_keys
                      lock_value unlock_value
                      lock_hash unlock_hash
-                     lock_keys_plus hash_locked
+                     lock_keys_plus
+                     hash_locked hash_unlocked
+                     hashref_locked hashref_unlocked
                      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
+                     lock_ref_keys_plus
                      hidden_ref_keys legal_ref_keys
-                     hv_store
 
+                     hash_seed hv_store
                     );
-    plan tests => 204 + @Exported_Funcs;
+    plan tests => 208 + @Exported_Funcs;
     use_ok 'Hash::Util', @Exported_Funcs;
 }
 foreach my $func (@Exported_Funcs) {
@@ -43,7 +47,7 @@ lock_keys(%hash);
 eval { $hash{baz} = 99; };
 like( $@, qr/^Attempt to access disallowed key 'baz' in a restricted hash/,
                                                        'lock_keys()');
-is( $hash{bar}, 23 );
+is( $hash{bar}, 23, '$hash{bar} == 23' );
 ok( !exists $hash{baz},'!exists $hash{baz}' );
 
 delete $hash{bar};
@@ -70,7 +74,7 @@ like( $@, qr/^Attempt to delete readonly key 'locked' from a restricted hash/,
 eval { $hash{locked} = 42; };
 like( $@, qr/^Modification of a read-only value attempted/,
                                            'trying to change a locked key' );
-is( $hash{locked}, 'yep' );
+is( $hash{locked}, 'yep', '$hash{locked} is yep' );
 
 eval { delete $hash{I_dont_exist} };
 like( $@, qr/^Attempt to delete disallowed key 'I_dont_exist' from a restricted hash/,
@@ -108,24 +112,23 @@ is( $hash{locked}, 42,  'unlock_value' );
     lock_value(%hash, 'RO');
 
     eval { %hash = (KEY => 1) };
-    like( $@, qr/^Attempt to delete readonly key 'RO' from a restricted hash/ );
+    like( $@, qr/^Attempt to delete readonly key 'RO' from a restricted hash/,
+        'attempt to delete readonly key from restricted hash' );
 }
 
 {
     my %hash = (KEY => 1, RO => 2);
     lock_keys(%hash);
     eval { %hash = (KEY => 1, RO => 2) };
-    is( $@, '');
+    is( $@, '', 'No error message, as expected');
 }
 
-
-
 {
     my %hash = ();
     lock_keys(%hash, qw(foo bar));
     is( keys %hash, 0,  'lock_keys() w/keyset shouldnt add new keys' );
     $hash{foo} = 42;
-    is( keys %hash, 1 );
+    is( keys %hash, 1, '1 element in hash' );
     eval { $hash{wibble} = 42 };
     like( $@, qr/^Attempt to access disallowed key 'wibble' in a restricted hash/,
                         'write threw error (locked)');
@@ -135,7 +138,6 @@ is( $hash{locked}, 42,  'unlock_value' );
     is( $@, '', 'unlock_keys' );
 }
 
-
 {
     my %hash = (foo => 42, bar => undef, baz => 0);
     lock_keys(%hash, qw(foo bar baz up down));
@@ -150,7 +152,6 @@ is( $hash{locked}, 42,  'unlock_value' );
           'locked "wibble"' );
 }
 
-
 {
     my %hash = (foo => 42, bar => undef);
     eval { lock_keys(%hash, qw(foo baz)); };
@@ -159,16 +160,19 @@ is( $hash{locked}, 42,  'unlock_value' );
                     'carp test' );
 }
 
-
 {
     my %hash = (foo => 42, bar => 23);
     lock_hash( %hash );
+    ok( hashref_locked( { %hash } ), 'hashref_locked' );
+    ok( hash_locked( %hash ), 'hash_locked' );
 
     ok( Internals::SvREADONLY(%hash),'Was locked %hash' );
     ok( Internals::SvREADONLY($hash{foo}),'Was locked $hash{foo}' );
     ok( Internals::SvREADONLY($hash{bar}),'Was locked $hash{bar}' );
 
     unlock_hash ( %hash );
+    ok( hashref_unlocked( { %hash } ), 'hashref_unlocked' );
+    ok( hash_unlocked( %hash ), 'hash_unlocked' );
 
     ok( !Internals::SvREADONLY(%hash),'Was unlocked %hash' );
     ok( !Internals::SvREADONLY($hash{foo}),'Was unlocked $hash{foo}' );
-- 
1.6.3.2

@p5pRT
Copy link
Author

p5pRT commented Apr 25, 2012

From @cpansprout

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

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2012

From @jkeenan

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

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2012

From @cpansprout

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

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2012

From @demerphq

On 26 April 2012 03​:13, James E Keenan via RT <perlbug-followup@​perl.org> 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.

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2012

From @cpansprout

On Thu Apr 26 00​:55​:53 2012, demerphq wrote​:

On 26 April 2012 03​:13, James E Keenan via RT <perlbug-
followup@​perl.org> 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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @jkeenan

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-archive.perl.org/perl5/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-archive.perl.org/perl5/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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @cpansprout

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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @doy

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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @jkeenan

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)

##########

Inline Patch
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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @cpansprout

On Sat Apr 28 19​:55​:04 2012, doy@​tozt.net 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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @cpansprout

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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @jkeenan

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@​cpan.org>
Date​: Sun Apr 29 07​:49​:51 2012 -0400

  Father C said switch the \!

Inline Patch
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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @jkeenan

On Sat Apr 28 21​:15​:41 2012, sprout wrote​:

On Sat Apr 28 19​:55​:04 2012, doy@​tozt.net 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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @jkeenan

lib/constant.pm
lib/Devel/PPPort.pm
lib/ExtUtils/Constant/ProxySubs.pm
lib/Hash/Util.pm
lib/Internals.t
lib/threads/shared.pm
lib/utf8.t
ext/Devel-Peek/Peek.xs
ext/Hash-Util/lib/Hash/Util.pm
ext/Hash-Util/t/Util.t
ext/mro/mro.xs
ext/Opcode/Opcode.xs
ext/PerlIO-scalar/scalar.xs
ext/PerlIO-scalar/t/scalar_ungetc.t
dist/constant/lib/constant.pm
dist/Cwd/ppport.h
dist/Storable/Storable.xs
dist/Storable/t/restrict.t
dist/threads/threads.xs
dist/threads-shared/lib/threads/shared.pm
dist/threads-shared/t/clone.t
cpan/Compress-Raw-Bzip2/Bzip2.xs
cpan/Compress-Raw-Zlib/Zlib.xs
cpan/DB_File/ppport.h
cpan/Devel-PPPort/ppport.h
cpan/Devel-PPPort/PPPort.pm
cpan/ExtUtils-Constant/lib/ExtUtils/Constant/ProxySubs.pm
cpan/IPC-SysV/ppport.h
cpan/List-Util/ListUtil.xs
cpan/Time-HiRes/ppport.h
cpan/Win32API-File/ppport.h

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @cpansprout

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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @cpansprout

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@​cpan.org>
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

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2012

From @cpansprout

Inline Patch
diff --git a/lib/Hash/Util.pm b/lib/Hash/Util.pm
index bb8a981..3eff6e7 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(@_) }
diff --git a/t/Util.t b/t/Util.t
index ba425de..2f39325 100644
--- a/t/Util.t
+++ b/t/Util.t
@@ -152,7 +152,7 @@ is( $hash{locked}, 42,  'unlock_value' );
 {
     my %hash = (foo => 42, bar => 23);
     lock_hash( %hash );
-    ok( hashref_locked( { %hash } ), 'hashref_locked' );
+    ok( hashref_locked( \%hash ), 'hashref_locked' );
     ok( hash_locked( %hash ), 'hash_locked' );
 
     ok( Internals::SvREADONLY(%hash),'Was locked %hash' );
@@ -175,7 +175,7 @@ TODO: {
     ok( ! hash_locked( %hash ), 'hash_locked negated' );
 
     lock_hash( %hash );
-    ok( ! hashref_unlocked( { %hash } ), 'hashref_unlocked negated' );
+    ok( ! hashref_unlocked( \%hash ), 'hashref_unlocked negated' );
     ok( ! hash_unlocked( %hash ), 'hash_unlocked negated' );
 }
 

@p5pRT
Copy link
Author

p5pRT commented Apr 30, 2012

From @jkeenan

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

@p5pRT
Copy link
Author

p5pRT commented Apr 30, 2012

From @jkeenan

0001-Document-hashref_locked-and-hashref_unlocked-.-Add-t.patch
From 90875ac8aee0f0eadb79109619e4598efabafa58 Mon Sep 17 00:00:00 2001
From: jkeenan <jkeenan@cpan.org>
Date: Sun, 29 Apr 2012 20:15:43 -0400
Subject: [PATCH] Document hashref_locked() and hashref_unlocked().  Add tests for them, include
 debugging by Father C++.

Make lock_hash_recurse() unlock_hash_recurse() exportable; include them in
SYNOPSIS; write tests for them.

Revise 'carp test' test. In general, tests of error messages should be written
with like() rather than is().  Why?  Because we rarely want to test for the
complete error message if that requires us to exactly calculate strings such
as the line number at which an error occurred.
---
 ext/Hash-Util/lib/Hash/Util.pm |   22 +++++++++----
 ext/Hash-Util/t/Util.t         |   68 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/ext/Hash-Util/lib/Hash/Util.pm b/ext/Hash-Util/lib/Hash/Util.pm
index dd2bb33..5075af3 100644
--- a/ext/Hash-Util/lib/Hash/Util.pm
+++ b/ext/Hash-Util/lib/Hash/Util.pm
@@ -29,6 +29,7 @@ our @EXPORT_OK  = qw(
                      hidden_ref_keys legal_ref_keys
 
                      hash_seed hv_store
+                     lock_hash_recurse unlock_hash_recurse
                     );
 our $VERSION = '0.11';
 require XSLoader;
@@ -72,6 +73,7 @@ Hash::Util - A selection of general-utility hash subroutines
                      hidden_ref_keys legal_ref_keys
 
                      hash_seed hv_store
+                     lock_hash_recurse unlock_hash_recurse
                    );
 
   %hash = (foo => 42, bar => 23);
@@ -142,8 +144,8 @@ the hash before you call lock_keys() so this shouldn't be a problem.
 
 Removes the restriction on the %hash's keyset.
 
-B<Note> that if any of the values of the hash have been locked they will not be unlocked
-after this sub executes.
+B<Note> that if any of the values of the hash have been locked they will not
+be unlocked after this sub executes.
 
 Both routines return a reference to the hash operated on.
 
@@ -314,9 +316,9 @@ lock_hash() locks an entire hash and any hashes it references recursively,
 making all keys and values read-only. No value can be changed, no keys can
 be added or deleted.
 
-B<Only> recurses into hashes that are referenced by another hash. Thus a
-Hash of Hashes (HoH) will all be restricted, but a Hash of Arrays of Hashes
-(HoAoH) will only have the top hash restricted.
+This method B<only> recurses into hashes that are referenced by another hash.
+Thus a Hash of Hashes (HoH) will all be restricted, but a Hash of Arrays of
+Hashes (HoAoH) will only have the top hash restricted.
 
     unlock_hash_recurse(%hash);
 
@@ -359,8 +361,11 @@ sub unlock_hashref_recurse {
 sub   lock_hash_recurse (\%) {   lock_hashref_recurse(@_) }
 sub unlock_hash_recurse (\%) { unlock_hashref_recurse(@_) }
 
+=item B<hashref_locked>
+
 =item B<hash_locked>
 
+  hashref_locked(\%hash) and print "Hash is locked!\n";
   hash_locked(%hash) and print "Hash is locked!\n";
 
 Returns true if the hash and its keys are locked.
@@ -369,13 +374,16 @@ 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(@_) }
 
+=item B<hashref_unlocked>
+
 =item B<hash_unlocked>
 
+  hashref_unlocked(\%hash) and print "Hash is unlocked!\n";
   hash_unlocked(%hash) and print "Hash is unlocked!\n";
 
 Returns true if the hash and its keys are unlocked.
@@ -384,7 +392,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(@_) }
diff --git a/ext/Hash-Util/t/Util.t b/ext/Hash-Util/t/Util.t
index fa0f66c..d02defe 100644
--- a/ext/Hash-Util/t/Util.t
+++ b/ext/Hash-Util/t/Util.t
@@ -34,8 +34,9 @@ BEGIN {
                      hidden_ref_keys legal_ref_keys
 
                      hash_seed hv_store
+                     lock_hash_recurse unlock_hash_recurse
                     );
-    plan tests => 208 + @Exported_Funcs;
+    plan tests => 226 + @Exported_Funcs;
     use_ok 'Hash::Util', @Exported_Funcs;
 }
 foreach my $func (@Exported_Funcs) {
@@ -155,15 +156,14 @@ is( $hash{locked}, 42,  'unlock_value' );
 {
     my %hash = (foo => 42, bar => undef);
     eval { lock_keys(%hash, qw(foo baz)); };
-    is( $@, sprintf("Hash has key 'bar' which is not in the new key ".
-                    "set at %s line %d.\n", __FILE__, __LINE__ - 2),
+    like( $@, qr/^Hash has key 'bar' which is not in the new key set/,
                     'carp test' );
 }
 
 {
     my %hash = (foo => 42, bar => 23);
     lock_hash( %hash );
-    ok( hashref_locked( { %hash } ), 'hashref_locked' );
+    ok( hashref_locked( \%hash ), 'hashref_locked' );
     ok( hash_locked( %hash ), 'hash_locked' );
 
     ok( Internals::SvREADONLY(%hash),'Was locked %hash' );
@@ -179,10 +179,23 @@ is( $hash{locked}, 42,  'unlock_value' );
     ok( !Internals::SvREADONLY($hash{bar}),'Was unlocked $hash{bar}' );
 }
 
+{
+    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' );
+}
 
 lock_keys(%ENV);
 eval { () = $ENV{I_DONT_EXIST} };
-like( $@, qr/^Attempt to access disallowed key 'I_DONT_EXIST' in a restricted hash/,   'locked %ENV');
+like(
+    $@,
+    qr/^Attempt to access disallowed key 'I_DONT_EXIST' in a restricted hash/,
+    'locked %ENV'
+);
 
 {
     my %hash;
@@ -444,6 +457,17 @@ ok($hash_seed >= 0, "hash_seed $hash_seed");
     is("@keys","0 2 4 6 8",'lock_ref_keys_plus() @keys DDS/t');
 }
 {
+    my %hash=(0..9, 'a' => 'alpha');
+    lock_ref_keys_plus(\%hash,'a'..'f');
+    ok(Internals::SvREADONLY(%hash),'lock_ref_keys_plus args overlap');
+    my @hidden=sort(hidden_keys(%hash));
+    my @legal=sort(legal_keys(%hash));
+    my @keys=sort(keys(%hash));
+    is("@hidden","b c d e f",'lock_ref_keys_plus() @hidden overlap');
+    is("@legal","0 2 4 6 8 a b c d e f",'lock_ref_keys_plus() @legal overlap');
+    is("@keys","0 2 4 6 8 a",'lock_ref_keys_plus() @keys overlap');
+}
+{
     my %hash=(0..9);
     lock_keys_plus(%hash,'a'..'f');
     ok(Internals::SvREADONLY(%hash),'lock_keys_plus args DDS/t');
@@ -454,6 +478,17 @@ ok($hash_seed >= 0, "hash_seed $hash_seed");
     is("@legal","0 2 4 6 8 a b c d e f",'lock_keys_plus() @legal DDS/t 3');
     is("@keys","0 2 4 6 8",'lock_keys_plus() @keys DDS/t 3');
 }
+{
+    my %hash=(0..9, 'a' => 'alpha');
+    lock_keys_plus(%hash,'a'..'f');
+    ok(Internals::SvREADONLY(%hash),'lock_keys_plus args overlap non-ref');
+    my @hidden=sort(hidden_keys(%hash));
+    my @legal=sort(legal_keys(%hash));
+    my @keys=sort(keys(%hash));
+    is("@hidden","b c d e f",'lock_keys_plus() @hidden overlap non-ref');
+    is("@legal","0 2 4 6 8 a b c d e f",'lock_keys_plus() @legal overlap non-ref');
+    is("@keys","0 2 4 6 8 a",'lock_keys_plus() @keys overlap non-ref');
+}
 
 {
     my %hash = ('a'..'f');
@@ -472,3 +507,26 @@ ok($hash_seed >= 0, "hash_seed $hash_seed");
     is_deeply(\@ph, \@bam, "Placeholders in place");
 }
 
+{
+    my %hash = (
+        a   => 'alpha',
+        b   => [ qw( beta gamma delta ) ],
+        c   => [ 'epsilon', { zeta => 'eta' }, ],
+        d   => { theta => 'iota' },
+    );
+    lock_hash_recurse(%hash);
+    ok( hash_locked(%hash),
+        "lock_hash_recurse(): top-level hash locked" );
+    ok( hash_locked(%{$hash{d}}),
+        "lock_hash_recurse(): element which is hashref locked" );
+    ok( ! hash_locked(%{$hash{c}[1]}),
+        "lock_hash_recurse(): element which is hashref in array ref not locked" );
+
+    unlock_hash_recurse(%hash);
+    ok( hash_unlocked(%hash),
+        "unlock_hash_recurse(): top-level hash unlocked" );
+    ok( hash_unlocked(%{$hash{d}}),
+        "unlock_hash_recurse(): element which is hashref unlocked" );
+    ok( hash_unlocked(%{$hash{c}[1]}),
+        "unlock_hash_recurse(): element which is hashref in array ref not locked" );
+}
-- 
1.6.3.2

@p5pRT
Copy link
Author

p5pRT commented May 8, 2012

From @nwc10

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

@p5pRT
Copy link
Author

p5pRT commented May 22, 2012

From @cpansprout

On Sun Apr 29 18​:30​:28 2012, jkeenan wrote​:

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.

Applied as 0857953 (Apr 22 patch) and 5114d26. Thank you.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented May 22, 2012

@cpansprout - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant