This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
Prune dead code in sv.c:sv_force_normal_flags
authorFather Chrysostomos <sprout@cpan.org>
Mon, 29 Oct 2012 06:47:22 +0000 (23:47 -0700)
committerFather Chrysostomos <sprout@cpan.org>
Tue, 30 Oct 2012 19:36:03 +0000 (12:36 -0700)
When a regexp is unregexped, a new SV (temp) is created, so it
can swap bodies with the regular expression (sv), and then temp
can be freed.

If SvLEN is 0, then a scalar does not own its string buffer.  Copied
regexps use that mechanism to share strings; only the original regexp
owns the string.

This little bit of code for handling the SvPVX field is strange:

/* Remember that SvPVX is in the head, not the body. */
if (SvLEN(temp)) {
    SvLEN_set(temp, SvLEN(sv));
    /* This signals "buffer is owned by someone else" in sv_clear,
       which is the least effort way to stop it freeing the buffer.
    */
    SvLEN_set(sv, SvLEN(sv)+1);
} else {
    /* Their buffer is already owned by someone else. */
    SvPVX(sv) = savepvn(SvPVX(sv), SvCUR(sv));
    SvLEN_set(temp, SvCUR(sv)+1);
}

Checking SvLEN(temp) is pointless if we have just created temp.  That
check is always false.  Presumably it was meant to be SvLEN(sv).  But
the original regexp scalar (i.e., not a copy) can never make it to
this function.  So SvLEN(sv) is always 0, which is why this has
not caused any problem.  The SvLEN_set inside the apodosis is also
strange.  ‘This signals "buffer is owned by someone else"’.  No it
certainly does not!  It is not setting SvLEN to 0, but definitely to
non-zero.  I can only assume this is a copy-and-paste error, which has
never caused a problem because it is unreachable.

I hereby excise this code (leaving the contents of the else).

sv.c

diff --git a/sv.c b/sv.c
index 2a4eb6b..7afc266 100644 (file)
--- a/sv.c
+++ b/sv.c
@@ -4797,17 +4797,10 @@ Perl_sv_force_normal_flags(pTHX_ register SV *const sv, const U32 flags)
        }
        SvCUR_set(temp, SvCUR(sv));
        /* Remember that SvPVX is in the head, not the body. */
-       if (SvLEN(temp)) {
-           SvLEN_set(temp, SvLEN(sv));
-           /* This signals "buffer is owned by someone else" in sv_clear,
-              which is the least effort way to stop it freeing the buffer.
-           */
-           SvLEN_set(sv, SvLEN(sv)+1);
-       } else {
-           /* Their buffer is already owned by someone else. */
-           SvPVX(sv) = savepvn(SvPVX(sv), SvCUR(sv));
-           SvLEN_set(temp, SvCUR(sv)+1);
-       }
+       assert(!SvLEN(sv));
+       /* Their buffer is already owned by someone else. */
+       SvPVX(sv) = savepvn(SvPVX(sv), SvCUR(sv));
+       SvLEN_set(temp, SvCUR(sv)+1);
 
        /* Now swap the rest of the bodies. */