Re: Factor out some common code of jmpenv blocks

On Sat, Jun 12, 2010 at 07:35:38PM +0200, Gerard Goossen wrote:
> On Sun, May 30, 2010 at 03:11:06PM +0100, Dave Mitchell wrote:
> > On Sat, May 29, 2010 at 08:40:07PM +0200, Gerard Goossen wrote:
> > > My fault, after rebasing I forgot to commit the latest changes 
> > > (they were already added, so git diff didn't show anything :( )
> > > Attached are the correct patches.
> > 
> > Having had a look at these changes, I'm concerned that although they are
> > advertised as just factoring out some common code, they actually change
> > execution behaviour. 
> 
> Advertising it at factoring out some common code, was wrong. It started
> out that way, but soon it also contained some minor (irrelevant) changes.
> The new attached patches, fixes the problem of calling it "factoring out"
> and also add some documentation to the new funtion. Below is also an
> explanation of why the changes.

But your patching structure still doesn't split out the "minor" changes
from the code reorganisation. I would expect a series of 1 or more patches
that strip out bits of redundant code (or whatever), together with
explanations in the commit messages of why they are redundant (e.g.
"the cleanup actions within my_exit_jump() have already been carried out
by the previous call to my_exit_jump(), so no need to run it again").

Finally, when all logical code changes have been done, the final commit
does the code shifting about.

I can't stress enough how important it is to arrange commits like this;
people like me spend endless hours reviewing old commits when trying to
understand and fix things. Having non-obvious code changes hidden away
is bad.

> > For example the code path in case 2 of call_sv() has
> > changed from:
> > 
> >     PL_curstash = PL_defstash;
> >     FREETMPS;
> >     JMPENV_POP;
> >     if (PL_e_script) {
> > 	SvREFCNT_dec(PL_e_script);
> > 	PL_e_script = NULL;
> >     }
> >     POPSTACK_TO(PL_mainstack);
> >     dounwind(-1);
> >     LEAVE_SCOPE(0);
> >     JMPENV_JUMP(2);
> > 
> > to
> > 
> >     PL_top_env = PL_top_env->je_prev;
> >     JMPENV_JUMP(2);
> > 
> > Now it may be that these excised steps are not needed, in which case I
> > would much prefer to see a separate commit which explicitly removes them
> > along with an explanation of why they aren't necessary.
> 
> Having an extra jmpenv level is something internal and shouldn't have any
> effect (it shouldn't matter whether code is executed usin eval_sv/call_sv or pp_entersub).
> Thus handling case 2 should just jump to the next level. That is the reason why
> the code is removed, the reason why the original code didn't do anything is
> because all that (poping the stack, unwinding, etc) is already done by the
> initial my_exit call.

But the FREETMPS above hasn't already been done. A quick run-through with
the debugger of 'BEGIN { exit 1; }' shows that at that point in call_sv(),
PL_tmps_ix == 7. So you are changing the behaviour of the code.

Note also that by manually expanding the JMPENV_POP macro in
Perl_runops_continue_from_jmpenv, you've lost the recent improvements to
the debugging output of that macro. It also means that when someone in
future does 'grep JMPENV_POP' on the source code, they'll miss your bit.

It would be better to keep using the macro, and fake up the cur_env; e.g.

    dJMPENV;
    ...

    cur_env = PL_top_env; /* fake up JMPENV_PUSH local variable */
    JMPENV_POP;
    JMPENV_JUMP(ret);


-- 
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"