develooper Front page | perl.perl5.porters | Postings from September 2021

Re: defined $a, where $a is actually a HASH

Thread Previous | Thread Next
From:
Nicholas Clark
Date:
September 8, 2021 09:57
Subject:
Re: defined $a, where $a is actually a HASH
Message ID:
YTiI3yFH/Qg3syao@etla.org
On Thu, Aug 19, 2021 at 05:18:10PM +0100, hv@crypt.org wrote:
> Nicholas Clark <nick@ccl4.org> wrote:
> :The summary - I think that this change is correct and should stay.
> :
> :
> :The details - I did this. It's in blead. The commit message explains the
> :reasoning. The code removed is deadweight, and likely contains 2 (more)
> :branches, which are one of the banes of performant code:
> [...]
> :    These have been unreachable since `defined @array` and `defined %hash`
> :    became syntax errors.
> 
> I think it would be wise to have an assert under DEBUGGING, especially
> since you have already discovered that they have _not_ been unreachable.

Good point. I did this:

commit 034242a8a539b024648b18318271eabddf40ca48
Author: Nicholas Clark <nick@ccl4.org>
Date:   Wed Sep 8 08:14:28 2021 +0000

    In pp_defined assert that the SV is not a hash or array.
    
    The code that handled hashes and arrays was removed by commit 2517717a8902:
        The cases for SVt_PVAV and SVt_PVHV in pp_defined are unreachable.
    
        Remove them, and hit to the C compiler that it's unlikely that someone used
        `defined` on a subroutine.
    
        These have been unreachable since `defined @array` and `defined %hash`
        became syntax errors. Whilst the same PP code is used for // and //=,
        expressions such as`@a // @b` put the left array (or hash) in scalar
        context, meaning that it always returns a define value.
        (Should we warn on these?)
    
    However, it turns out that that the removed code was reachable by XS code
    generating data structures that would be "illegal" in pure Perl (eg also
    triggering "Bizarre copy of ..." errors). Hence with -DDEBUGGING, add logic
    to report problematic cases, instead of silently continuing with changed
    behaviour.

diff --git a/pp_hot.c b/pp_hot.c
index 17683ff000..43c61bd272 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -1365,6 +1365,15 @@ PP(pp_defined)
             RETSETNO;
     }
 
+    /* Historically what followed was a switch on SvTYPE(sv), handling SVt_PVAV,
+     * SVt_PVCV, SVt_PVHV and "default". `defined &sub` is still valid syntax,
+     * hence we still need the special case PVCV code. But AVs and HVs now
+     * should never arrive here... */
+#ifdef DEBUGGING
+    assert(SvTYPE(sv) != SVt_PVAV);
+    assert(SvTYPE(sv) != SVt_PVHV);
+#endif
+
     if (UNLIKELY(SvTYPE(sv) == SVt_PVCV)) {
         if (CvROOT(sv) || CvXSUB(sv))
             defined = TRUE;


Nicholas Clark

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