Remove Devel::BeginLift

[schwern]

This patch series eliminates Devel::BeginLift per #39.

Instead of using BeginLift it simply rewrites "method foo {...}" as "method &foo; sub foo { ... }".

Tested with Strawberry Perl 5.16.1 64 bit. That's what I have on my VM. Needs more Windows testing, but it can't be any worse than the shape we're currently in.

[barefootcoder]

Okay, I've had a chance to go over the code more closely now. Here are the comments I have:

• The comment for inject_if_block says: For instance, if it's an anonymous sub, the $before parameter would contain "sub ". (And, yes, I know you didn't write that--I did. But that may have been due to an incomplete understanding on my part.) But the parser from Devel::Declare::MethodInstaller::Simple seems to determine whether or not to provide a $before argument not based on whether or not the sub is anonymous, but rather whether or not it has attributes. Now, I'm not entirely sure why this is, but would it matter at all to the way you've implemented inject_if_block in MS?
• You're ignoring (effectively throwing away) the $before argument for all named subs. Particularly in the case of MSM, which is slapping in a 'sub ' in all cases, throwing away $before seems bad. Of course, I'm starting to suspect that MSM's inject_if_block is no longer necessary with the code you've got here--I'm going to test that on a few different versions of Perl before I change it though--so perhaps that's moot. But there's still parser from DDMIS, which will set $before in the presence of attributes. I suppose you're getting around that by setting the attributes yourself via $self->{attributes}, but still ... this seems like one of those things that may conveniently work today, but may give us troubles tomorrow if someone changes something upstream ... or maybe downstream, if someone overrides something and passes us a $before that we then toss. Not entirely sure what the right answer here is, but I wanted to throw it out there so we were aware of it.

And I think that's all I can see that needs your counter-comment. As I say, next I will set up a little perlbrew farm and test this on some different Perl versions. If that all looks good, I'm going to play around with removing the inject_if_block from MSM (it almost surely is redundant at this point). And if that looks good, and you have no worries from my comments above, I'll just go ahead and merge the branch, turn it into a dev version, and set CPAN Testers on it.

Thoughts?

[schwern]

Sorry I dropped off the Internet for a while.

I was confused about how that works, too, but I just figured it out and found
a bug. Take this...

my $foo = func { ... };

it compiles into something like this...

sub func(&) { return shift; };
my $foo = func { ... };

func() has a & prototype, so there's no need for the "sub" part. It would
seem to be just a kinda unnecessary optimization to remove the sub, except it
has subtle effects on how the prototype is parsed. These are revealed by the
larna.t tests.

sub func(&) { return shift; };
my $a = [ func { 123 }, 1 ];

sub func(&) { return shift; };
my $a = [ func sub { 123 }, 1 ];

The top one works, the bottom one fails "too many arguments for main::func".
The prototype must be getting confused. This may be why it fixed a 5.10 bug,
there may have been a prototype glitch. Or maybe this behavior is a prototype
glitch. Either way it works now on 5.10.0 and 5.14.1.

The bug it revealed is I got the logic backwards, so the code wasn't applying
to anonymous routines. So you're right that I was throwing away the $before
argument for all named subs. Devel::Declare was taking care of it, because
$before was false so it handled it itself. There's no need to replicate their
functionality at all! inject_if_block() now just adds our BEGIN functionality
only for named subroutines when compile_at_BEGIN is on.

[barefootcoder]

Okay, I mostly followed all that. I guess my main question, though, is why do we need

sub func(&) { return shift; };
my $foo = func { ... };

when it seems like

my $foo = sub { ... };

would work just as well? And also not engender the problem you describe from larna.t ...

Still planning on jumping on merging-and-releasing this branch next, BTW. JIC you thought I was falling down on the job over here. :-D

[schwern]

On 2012.10.26 8:11 PM, Buddy Burden wrote:

Okay, I mostly followed all that. I guess my main question, though, is why do
we /need/

|sub func(&) { return shift; };
my $foo = func { ... };

when it seems like

my $foo = sub { ... };

would work just as well? And also not engender the problem you describe from
larna.t ...

You'll have to ask the DD folks about that. I suspect its because DD stops
the parser just after the "func" or "method" keyword, so it has to do
something with it. It might be an artifact of earlier DD versions they never
got rid of. I've tried several times to hack up DD to get rid of it and failed.

Still planning on jumping on merging-and-releasing this branch next, BTW. JIC
you thought I was falling down on the job over here. :-D

Nope, you seem to be well on top of things.

1. Not allowed to add "In accordance with the prophesy" to the end of
   answers I give to a question an officer asks me.
   -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
   http://skippyslist.com/list/

[barefootcoder]

Seems to work on all Perl's I tried it on (5.8.9, 5.10.1, 5.12.4, 5.14.2, 5.16.1). The change you made now makes it so that the inject_if_block in MSM is no longer redundant, so that will be staying. I'm ready to merge this in, although I'm still trying to figure out why I haven't seen any CPANTesters reports on the last dev release yet. But I may just go ahead and whip out another one and see if that one fares any better.