This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
op.c:ck_grep: Remove unnecessary op_next assignments
authorFather Chrysostomos <sprout@cpan.org>
Sat, 14 Jul 2012 20:28:57 +0000 (13:28 -0700)
committerFather Chrysostomos <sprout@cpan.org>
Sun, 15 Jul 2012 00:35:32 +0000 (17:35 -0700)
ck_grep is a tangled mess and I am trying to simplify it, but at each
step it is not at all obvious what is happening.

grep/map with a block starts out like this:

    grepstart
      pushmark
      null
        scope/leave
          block contents...
      arg1
      arg2
      ...

The if (o->op_flags & OPf_STACKED) at the top of ck_grep runs for
blocks.  The firstkid var refers to the null above, and gets its
op_next set (this code was in ck_sort until recently [ck_grep used to
call ck_sort], and this particular op_next assignment was specific to
grep and map).

Later, the thing is fed through ck_fun, whose OA_CVREF case transforms
the tree into this:

    grepstart
      pushmark
      null             ← new op
        null           ← original null
          scope/leave
            block contents...
      arg1
      arg2
      ...

Then cUNOPx(cLISTOP->op_first->op_sibling)->op_first gets its op_next
set.  By that point, that expression refers to the original null op,
so setting its op_next pointer to begin with seems pointless.

But it is not actually pointless.  Removing that statement breaks
things.  It turns out, as usual, that everything is more complicated
than it seems.  Setting kid->op_next = (OP*)gwop in the block-specific
if statement after setting the first null op’s op_next pointer results
in op_next pointers like this (the newly-created grepwhile doesn’t
link to the grepstart yet at this point):

  [grepwhile]
    grepstart
      pushmark
      null              -> block contents
        scope/leave     -> grepwhile
          block contents...
      arg1
      arg2
      ...

So ck_fun’s OA_CVREF handling, when it calls LINKLIST, sees that the
original null op already has its op_next set and does nothing.  With-
out it set, it copies it from the first kid, resulting in this (when
LINKLIST copies the op_next pointer out, it makes the kid it was cop-
ied from point to its sibling or its parent):

  [grepwhile]
    grepstart
      pushmark
      null
        null              -> grepwhile
          scope/leave     -> null (prev line)
            block contents...
      arg1
      arg2
      ...

That nonsensical arrangement of op_next pointers basically prevents
the block from running, because the grepwhile’s other pointer, copied
from the first null’s kid, ends up pointing to the grepwhile itself.

If we also remove the kid->op_next = (OP*)gwop assignment from the
if(OPf_STACKED) block, then we end up with this after ck_fun:

  [grepwhile]
    grepstart
      pushmark
      null
        null              -> first op in block
          scope/leave     -> null (prev line)
            block contents...
      arg1
      arg2
      ...

Then the op_next poiner from the first null’s kid is copied to
grepwhile’s op_other pointer, and everything works.

This also means we can remove the now-redundant LINKLIST call from the
if(OPfSTACKED) block, since ck_fun’s OA_CVREF handling also does that.
So now every vestige of the original call to ck_sort is gone.

This also means we no longer have to repeat NewOp(1101, gwop...).

op.c

diff --git a/op.c b/op.c
index b5a78d5..b5761f7 100644 (file)
--- a/op.c
+++ b/op.c
@@ -8569,7 +8569,7 @@ OP *
 Perl_ck_grep(pTHX_ OP *o)
 {
     dVAR;
-    LOGOP *gwop = NULL;
+    LOGOP *gwop;
     OP *kid;
     const OPCODE type = o->op_type == OP_GREPSTART ? OP_GREPWHILE : OP_MAPWHILE;
     PADOFFSET offset;
@@ -8580,14 +8580,9 @@ Perl_ck_grep(pTHX_ OP *o)
     /* don't allocate gwop here, as we may leak it if PL_parser->error_count > 0 */
 
     if (o->op_flags & OPf_STACKED) {
-       OP *firstkid = cLISTOPo->op_first->op_sibling;
-        kid = cUNOPx(firstkid)->op_first;
+        kid = cUNOPx(cLISTOPo->op_first->op_sibling)->op_first;
        if (kid->op_type != OP_SCOPE && kid->op_type != OP_LEAVE)
            return no_fh_allowed(o);
-       LINKLIST(kid);
-       firstkid->op_next = kLISTOP->op_first;
-       NewOp(1101, gwop, 1, LOGOP);
-       kid->op_next = (OP*)gwop;
        o->op_flags &= ~OPf_STACKED;
     }
     kid = cLISTOPo->op_first->op_sibling;
@@ -8603,8 +8598,7 @@ Perl_ck_grep(pTHX_ OP *o)
        Perl_croak(aTHX_ "panic: ck_grep, type=%u", (unsigned) kid->op_type);
     kid = kUNOP->op_first;
 
-    if (!gwop)
-       NewOp(1101, gwop, 1, LOGOP);
+    NewOp(1101, gwop, 1, LOGOP);
     gwop->op_type = type;
     gwop->op_ppaddr = PL_ppaddr[type];
     gwop->op_first = listkids(o);