Bug 74836

Created attachment 119810 [details] Patch

Comment on attachment 119810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119810&action=review > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:161 > +my $actualOutput = LoadAsModule::parseLeaksOutput(\@input); By the way, I am not sure why we need to replace @input with \@input. Reduced test case: sub f(\@){ my $p = shift; print "@$p\n";} my @v=(1, 2, 3); f(@v); # This works. eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}'; my @vv=(1, 2, 3); ff(@vv); # This does not work. ff(\@vv); # This works instead. Why???

Created attachment 119811 [details] Patch

(In reply to comment #2) > (From update of attachment 119810 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119810&action=review > > > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:161 > > +my $actualOutput = LoadAsModule::parseLeaksOutput(\@input); > > By the way, I am not sure why we need to replace @input with \@input. > > Reduced test case: > > sub f(\@){ my $p = shift; print "@$p\n";} > my @v=(1, 2, 3); > f(@v); # This works. > > eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}'; > my @vv=(1, 2, 3); > ff(@vv); # This does not work. > ff(\@vv); # This works instead. Why??? It's because Perl doesn't see a prototype for ff(), so it defaults to the no-prototype behavior. You can fix it by doing this (although I'm not sure this is a good solution for generalizing the LoadAsModule.pm code (will look at the patch next): sub ff(\@); eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}'; my @vv=(1, 2, 3); ff(@vv); # This now works. # ff(\@vv); # This now fails with a compilation error (Type of arg 1 to main::ff must be array (not reference constructor)) See the "Prototypes" section of "man perlsub" for more details.

(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 119810 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=119810&action=review > > eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}'; > > my @vv=(1, 2, 3); > > ff(@vv); # This does not work. > > ff(\@vv); # This works instead. Why??? > > It's because Perl doesn't see a prototype for ff(), so it defaults to the no-prototype behavior. You can fix it by doing this (although I'm not sure this is a good solution for generalizing the LoadAsModule.pm code (will look at the patch next): > > sub ff(\@); > eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}'; > my @vv=(1, 2, 3); > ff(@vv); # This now works. > # ff(\@vv); # This now fails with a compilation error (Type of arg 1 to main::ff must be array (not reference constructor)) ddkilzer: Thanks. However, actually, we cannot write the prototype like 'sub ff(\@)' in LoadAsModule.pm, because we cannot easily know what prototypes will appear in the loading Perl script. So the possible options would be [a] Use \@input instead of @input (i.e. this patch). [b] Leave RunLeaks.pm as it is, and we create PrepareChangeLog.pm. PrepareChangeLog.pm will be almost a copy of RunLeaks.pm. > although I'm not sure this is a good solution for generalizing the LoadAsModule.pm code Do you prefer [b]?

Comment on attachment 119811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119811&action=review > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:36 > -use RunLeaks; > use Test::More; > +use lib ".."; > +use LoadAsModule; > + > +LoadAsModule::load("run-leaks"); I think this is definitely a step in the right direction (sharing code to load Perl scripts as modules), but this implementation has a couple of drawbacks: 1. You need a separate LoadAsModule::load() call for each script. 2. Scripts to test are in the LoadAsModule package, not their own package. 3. If two scripts had to be loaded, any methods with the same name would collide. I think LoadAsModule should work like this where the first argument is the package name, and the second is the script name: use lib ".."; use LoadAsModule qw(RunLeaks run-leaks); There is a special "import" method that you may define that gets called when there are arguments to the 'use' statement (see lib.pm for an example). Maybe we can put the whole sub-package in a big eval statement? package LoadAsModule; [...] sub import { my ($self, $package, $script) = @_; eval q{ package } . $package . q{; use [...]; sub readFile($); my $runLeaksPath = File::Spec->catfile(sourceDir(), "Tools", "Scripts", "} . $script . q{"); eval "sub {" . readFile($runLeaksPath) . "}"; sub readFile($) { [...] }; } I'm not sure yet if this would work or not, though. Maybe we can just do something more straight-forward without using a giant eval statement. I'm going to play with this a bit. > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-new.pl:128 > -is_deeply($actualOutput, $expectedOutput, "leaks Report Version 2.0 (old)"); > +is_deeply($actualOutput, $expectedOutput, "leaks Report Version 2.0 (new)"); Nice catch!

(In reply to comment #6) > use LoadAsModule qw(RunLeaks run-leaks); I've got this working! > Maybe we can just do something more straight-forward without using a giant eval statement. I'm going to play with this a bit. I'll post a patch or the new LoadAsModule.pm in a few minutes.

Created attachment 119929 [details] How to use import and eval in LoadAsModule.pm

Comment on attachment 119811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119811&action=review >> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:36 >> +LoadAsModule::load("run-leaks"); > > I think this is definitely a step in the right direction (sharing code to load Perl scripts as modules), but this implementation has a couple of drawbacks: > > 1. You need a separate LoadAsModule::load() call for each script. > 2. Scripts to test are in the LoadAsModule package, not their own package. > 3. If two scripts had to be loaded, any methods with the same name would collide. > > I think LoadAsModule should work like this where the first argument is the package name, and the second is the script name: > > use lib ".."; > use LoadAsModule qw(RunLeaks run-leaks); > > There is a special "import" method that you may define that gets called when there are arguments to the 'use' statement (see lib.pm for an example). > > Maybe we can put the whole sub-package in a big eval statement? > > package LoadAsModule; > > [...] > > sub import > { > my ($self, $package, $script) = @_; > > eval q{ > package } . $package . q{; > > use [...]; > > sub readFile($); > > my $runLeaksPath = File::Spec->catfile(sourceDir(), "Tools", "Scripts", "} . $script . q{"); > eval "sub {" . readFile($runLeaksPath) . "}"; > > sub readFile($) { > [...] > }; > } > > I'm not sure yet if this would work or not, though. > > Maybe we can just do something more straight-forward without using a giant eval statement. I'm going to play with this a bit. See the attached patch for how to do this.

Created attachment 119935 [details] rebased patch for review

Comment on attachment 119935 [details] rebased patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=119935&action=review r=me > Tools/Scripts/webkitperl/LoadAsModule.pm:35 > +use lib File::Spec->catdir($FindBin::Bin, "..", ".."); Are you sure you need two ".." arguments here? It worked with just one ".." argument for me running: ./Tools/Scripts/test-webkitperl

(In reply to comment #11) > (From update of attachment 119935 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119935&action=review > > r=me > > > Tools/Scripts/webkitperl/LoadAsModule.pm:35 > > +use lib File::Spec->catdir($FindBin::Bin, "..", ".."); > > Are you sure you need two ".." arguments here? It worked with just one ".." argument for me running: > > ./Tools/Scripts/test-webkitperl One ".." is enough to run ./Tools/Scripts/test-webkitperl, but two ".."s are needed to run ./Tools/Scripts/test-webkitperl and 'perl ./Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl' (although I am not sure we need to support the latter). Are there any concern with two ".."s?

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 119935 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=119935&action=review > > > > r=me > > > > > Tools/Scripts/webkitperl/LoadAsModule.pm:35 > > > +use lib File::Spec->catdir($FindBin::Bin, "..", ".."); > > > > Are you sure you need two ".." arguments here? It worked with just one ".." argument for me running: > > > > ./Tools/Scripts/test-webkitperl > > One ".." is enough to run ./Tools/Scripts/test-webkitperl, but two ".."s are needed to run ./Tools/Scripts/test-webkitperl and 'perl ./Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl' (although I am not sure we need to support the latter). > > Are there any concern with two ".."s? As long as ./Tools/Scripts/test-webkitperl works with two ".." arguments, that's fine.

Comment on attachment 119935 [details] rebased patch for review Clearing flags on attachment: 119935 Committed r103338: <http://trac.webkit.org/changeset/103338>

All reviewed patches have been landed. Closing bug.