Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use Devel::LexAlias #3

Closed
wants to merge 8 commits into from
Closed

use Devel::LexAlias #3

wants to merge 8 commits into from

Conversation

tobyink
Copy link
Contributor

@tobyink tobyink commented Jul 25, 2013

This is a second attempt at a fix for #2 using Devel::LexAlias.

It has a fallback to using package variables when Devel::LexAlias is not available, because I think it's nice to have a pure Perl implementation. The two code paths only diverge in a couple of very small places, so I don't think it should have much impact on Eval::Closure's maintainability.

@doy
Copy link
Owner

doy commented Jul 25, 2013

I still don't like the global variable idea. For instance:

#!/usr/bin/env perl
use strict;
use warnings;
use Test::More;

use Eval::Closure;

my $destroyed = 0;

package Foo {
    sub DESTROY { $destroyed++ }
}

{
    my $number  = bless \(my $foo), "Foo";
    $$number = 40;
    my $closure = eval_closure(
            source       => 'sub { $$xxx += 2 }',
            environment  => { '$xxx' => \$number },
    );

    $closure->();

    is($$number, 42);
    is($destroyed, 0);
}
is($destroyed, 1);

done_testing;

There are a lot of ways that global variables don't behave the same way as lexical variables do, and I don't want to have to deal with all of that and end up breaking in a bunch of different edge cases.

@tobyink
Copy link
Contributor Author

tobyink commented Jul 25, 2013

Hmmm... good point.

@tobyink
Copy link
Contributor Author

tobyink commented Jul 25, 2013

I do actually have another a pure Perl fallback solution (using tied lexicals) if you're interested.

@doy
Copy link
Owner

doy commented Jul 25, 2013

Not particularly, that would be even more broken (you wouldn't be able to close over existing tied variables anymore, for instance). Maybe aliasing should just be an option, that dies if Devel::LexAlias isn't installed?

@tobyink
Copy link
Contributor Author

tobyink commented Jul 25, 2013

I have a test case with a tied scalar that works. I'm not doing anything especially interesting in the tied object; just:

my @store;

{
    package MyTie;
    use Tie::Scalar ();
    our @ISA = 'Tie::StdScalar';
    sub STORE {
        my $self = shift;
        push @store, $_[0];
        $self->SUPER::STORE(@_);
    }
}

@tobyink
Copy link
Contributor Author

tobyink commented Jul 25, 2013

The only place it fails is if you call tied($closed_over), you get back the hacky-hack tied object rather than getting back what you should get back.

@doy
Copy link
Owner

doy commented Jul 25, 2013

On Thu, Jul 25, 2013 at 12:20:47PM -0700, Toby Inkster wrote:

The only place it fails is if you call tied($closed_over), you get
back the hacky-hack tied object rather than getting back what you
should get back.

I imagine it'll also fail if you untie the original variable outside of
the closure, or tie it to something else.

Basically, if this is going to be an implicit decision based only on
whether or not Devel::LexAlias is installed, it has to either produce
identical results in all cases, or die. I really don't want "almost the
same, except breaks in edge cases" when running identical code in
different environments.

@tobyink
Copy link
Contributor Author

tobyink commented Jul 25, 2013

I imagine it'll also fail if you untie the original variable outside of the closure, or tie it to something else.

No, that all works fine.

@doy
Copy link
Owner

doy commented Jul 26, 2013

My second point still stands.

@tobyink
Copy link
Contributor Author

tobyink commented Jul 26, 2013

This is all quite academic anyway - the pull request as it stands doesn't include the fallback to tie.

@doy
Copy link
Owner

doy commented Jul 29, 2013

Thinking about this some more, I think this should probably be an alias => 1 option rather than default behavior, at least for now. This preserves backwards compatibility, and allows the module to remain pure-perl by default (the alias option should start by doing something like die unless eval { require Devel::LexAlias }).

@tobyink
Copy link
Contributor Author

tobyink commented Jul 29, 2013

OK; I'll sort that out tomorrow.

@tobyink
Copy link
Contributor Author

tobyink commented Jul 30, 2013

Done.

@doy
Copy link
Owner

doy commented Jul 30, 2013

Thanks, that looks good.

@doy doy closed this Jul 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants