develooper Front page | perl.perl5.porters | Postings from April 2012

Re: [perl #112126] Hash::Util export bug

Thread Previous | Thread Next
From:
Jesse Luehrs
Date:
April 28, 2012 19:54
Subject:
Re: [perl #112126] Hash::Util export bug
Message ID:
20120429025431.GT4502@tozt.net
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

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About