Radoslaw Zielinski writes: > [EMAIL PROTECTED] <[EMAIL PROTECTED]> [14-07-2006 12:34]: > > Author: jm > > Date: Fri Jul 14 03:34:29 2006 > > New Revision: 421862 > > > URL: http://svn.apache.org/viewvc?rev=421862&view=rev > > Log: > > bug 4777: evalstr version of run_eval_tests, thanks Michael > > > Modified: > > spamassassin/trunk/lib/Mail/SpamAssassin/PerMsgStatus.pm > > Whoa, that's a hackish one. Wouldn't storing references to anonymous > subs be better (maybe with help of an AUTOLOAD)? Modifying namespace > at run time is sooo far from OO... And situation where foo->can("x") > is true, but foo->x() throws an exception... That's just plain wrong.
Yep -- it would be nicer. Sadly, last time I checked, it was faster this way, and that's the #1 priority for these methods. The overhead of dereferencing the subroutine is just enough to cause a slowdown. For a demonstration, see "bench_subs" at http://taint.org/xfer/2006/bench_subs.txt -- : jm 125...; ~/ftp/bench/bench_subs Rate sub_ref evaled_sub sub_ref 568181/s -- -11% evaled_sub 641024/s 13% -- 4499991 4499991 Now, these subs aren't performance-critical, so it would be possible to move to code refs for *these* functions, but leave the body subs (which are generated from the body ruleset and are the most speed-critical part) as namespace-modifying functions. But then we'd have 2 separate methods of doing the same thing and we'd still need that namespace-cleanup code; in my opinion, better to just have just one kludgy method, instead of one elegant *and* one kludgy one. ;) > [...] > > + foreach my $method (@TEMPORARY_METHODS) { > > + if (defined &{$method}) { > > + undef &{$method}; > > + } > > + } > > Shouldn't it be cleared later? > > @TEMPORARY_METHODS = (); Uh, yeah. fixed! > [...] > > + # Some of the rules are scoreset specific, so we need additional > > + # subroutines to handle those > > + if (defined &{'Mail::SpamAssassin::PerMsgStatus::'.$methodname} > > Prettier (IMHO): > > if (Mail::SpamAssassin::PerMsgStatus->can($methodname) Disappointingly, that's a defined test for a reason. can() and defined() don't always produce the same output! That's why there's this double-test at line 2901: # return if it's been registered already return if ($self->can ($function) && defined &{'Mail::SpamAssassin::PerMsgStatus::'.$function}); > > + && !$doing_user_rules) > > + { > > + no strict "refs"; > > + > > &{'Mail::SpamAssassin::PerMsgStatus::'.$methodname}($self,@extraevalargs); > > + use strict "refs"; > > No need for 'no strict': > > my $method = 'Mail::SpamAssassin::PerMsgStatus::' . $methodname; > $self->$method(@extraevalargs); Sorry -- I'd need to see more platform checks to ensure that really *is* a safe change now; it was added years ago by Matt for a reason, as I recall. > [...] > > + $evalstr = <<"EOT"; > > +{ > > + package Mail::SpamAssassin::PerMsgStatus; > > + > > + sub ${methodname} { > > Less confusing: > > sub Mail::SpamAssassin::PerMsgStatus::$methodname { > > [...] Not equivalent; the latter does not have access to package-global vars in Mail::SpamAssassin::PerMsgStatus scope without the "package" line. see: perl -we ' use strict; package Foo; { our $a = 2; } package Foo; sub run1 { print "package Foo; run1 $a\n"; } package main; sub Foo::run2 { print "Foo::run2 $a\n"; } package main; Foo::run1(); Foo::run2(); ' Name "main::a" used only once: possible typo at -e line 5. package Foo; run1 2 Use of uninitialized value in concatenation (.) or string at -e line 5. Foo::run2 IOW, the sub has been placed in the package, but references to variables use the outer package scope, as far as I can see. > > + no strict "refs"; > > + > > &{'Mail::SpamAssassin::PerMsgStatus::'.$methodname}($self,@extraevalargs); > > + use strict "refs"; > > Same as above. --j.
