Michael_Schilli via RT wrote:
Show quoted text>> and I came across an inconsistency in
>> Log::Log4perl::Logger::callerline().
>
> I fiddled with that particular code for a long time until I got it
> working. The reason for the stack scanning is, IIRC, that it's used from
> both within Log4perl and outside and there's no easy way to determine
> the depth (or so I thought at the time). If you have an idea on how to
> replace it with caller_depth, feel free to play with it, the test suite
> will complain if there's a mistake. Patches are definitely welcome.
Simply replacing it with what you suggest in the comments seems to work.
I read and re-read 024WarnDieCarp.t, and the tests there should be
catching any problems. Yet everything passes with flying colors.
Also the first line of callerline() was:
my($message) = @_;
which I changed to
my $message = join ('', @_);
for consistency with the rest of the interfaces. Ignore this change if
it breaks some legacy behavior (doubt it)
Show quoted text>> Additionally I noticed that in and_warn() and and_die() and at some
>> other places you use ...(@_[0 .. $#_]), which is an extremely elaborate
>> way to say @_. What is the reason for this?
>
> It's indeed the same thing, the reason for this is most likely old code
> that had a different range but got changed -- should be changed to @_
> for better readability.
Fixed.
Also I made some (dubious) optimizations by replacing all possible
local() invocations by $var++/$var-- whenever this is possible. This
simple proggy shows that local is 10% slower. Not that it really matters
but we are shaving CPU cycles right? :)
use Benchmark qw/cmpthese/;
our $x = 3;
cmpthese (-3, {
inc_dec => sub {
++$x;
do {1} for (1 .. 10);
--$x;
},
localized => sub {
local $x = $x + 1;
do {1} for (1 .. 10);
},
});
Hope you find the patch useful.
Cheers!
Peter
? eg/benchmarks/incdec_vs_local
Index: lib/Log/Log4perl.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl.pm,v
retrieving revision 1.241
diff -u -r1.241 Log4perl.pm
--- lib/Log/Log4perl.pm 20 Aug 2008 01:22:25 -0000 1.241
+++ lib/Log/Log4perl.pm 21 Aug 2008 01:30:26 -0000
@@ -100,8 +100,6 @@
if(exists $tags{':levels'}) {
# Export log levels ($DEBUG, $INFO etc.) from Log4perl::Level
- my $caller_pkg = caller();
-
for my $key (keys %Log::Log4perl::Level::PRIORITY) {
my $name = "$caller_pkg\::$key";
# Need to split this up in two lines, or CVS will
@@ -354,12 +352,12 @@
if(@_ == 0) {
# 1
- $category = scalar caller();
+ $category = scalar caller($Log::Log4perl::caller_depth);
} elsif(@_ == 1) {
# 2, 3
if($_[0] eq __PACKAGE__) {
# 2
- $category = scalar caller();
+ $category = scalar caller($Log::Log4perl::caller_depth);
} else {
$category = $_[0];
}
Index: lib/Log/Log4perl/Logger.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl/Logger.pm,v
retrieving revision 1.88
diff -u -r1.88 Logger.pm
--- lib/Log/Log4perl/Logger.pm 20 Aug 2008 01:16:21 -0000 1.88
+++ lib/Log/Log4perl/Logger.pm 21 Aug 2008 01:30:29 -0000
@@ -833,30 +833,20 @@
# call me from a sub-func to spew the sub-func's caller
#######################################################
sub callerline {
- # the below could all be just:
- # my ($pack, $file, $line) = caller(2);
- # but if we every bury this further, it'll break. So we do this
- # little trick stolen and paraphrased from Carp/Heavy.pm
- my($message) = @_;
-
- my $i = 0;
- my (undef, $localfile, undef) = caller($i++);
- my ($pack, $file, $line);
- do {
- ($pack, $file, $line) = caller($i++);
- } while ($file && $file eq $localfile);
-
- my $has_newline;
-
- $has_newline++ if chomp $message;
-
- $message .= " at $file line $line" if !$has_newline;
-
- # Someday, we'll use Threads. Really.
- if (defined &Thread::tid) {
- my $tid = Thread->self->tid;
- $message .= " thread $tid" if $tid and !$has_newline;
+ my $message = join ('', @_);
+
+ my ($pack, $file, $line) = caller($Log::Log4perl::caller_depth + 1);
+
+ if (not chomp $message) { # no newline
+ $message .= " at $file line $line";
+
+ # Someday, we'll use Threads. Really.
+ if (defined &Thread::tid) {
+ my $tid = Thread->self->tid;
+ $message .= " thread $tid" if $tid;
+ }
}
+
return ($message, "\n");
}
@@ -864,15 +854,14 @@
sub and_warn {
#######################################################
my $self = shift;
- my $msg = join("", @_[0 .. $#_]);
- CORE::warn(callerline($self->warning_render(@_[0 .. $#_])));
+ CORE::warn(callerline($self->warning_render(@_)));
}
#######################################################
sub and_die {
#######################################################
my $self = shift;
- die(callerline($self->warning_render(@_[0 .. $#_])));
+ die(callerline($self->warning_render(@_)));
}
##################################################
@@ -936,11 +925,11 @@
if ($self->is_warn()) {
my $message = Carp::longmess($msg);
- local $Log::Log4perl::caller_depth =
- $Log::Log4perl::caller_depth + 1;
+ $Log::Log4perl::caller_depth++;
foreach (split(/\n/, $message)) {
$self->warn("$_\n");
}
+ $Log::Log4perl::caller_depth--;
Carp::cluck($msg);
}
}
@@ -955,11 +944,11 @@
if ($self->is_warn()) {
my $message = Carp::shortmess($msg);
- local $Log::Log4perl::caller_depth =
- $Log::Log4perl::caller_depth + 1;
+ $Log::Log4perl::caller_depth++;
foreach (split(/\n/, $message)) {
$self->warn("$_\n");
}
+ $Log::Log4perl::caller_depth--;
Carp::carp($msg) if $Log::Log4perl::LOGDIE_MESSAGE_ON_STDERR;
}
}
@@ -974,13 +963,13 @@
local $Carp::CarpLevel = $Carp::CarpLevel + 1;
my $msg = $self->warning_render(@_);
- my $message = Carp::shortmess($msg);
- local $Log::Log4perl::caller_depth =
- $Log::Log4perl::caller_depth + 1;
if ($self->is_fatal()) {
+ my $message = Carp::shortmess($msg);
+ $Log::Log4perl::caller_depth++;
foreach (split(/\n/, $message)) {
$self->fatal("$_\n");
}
+ $Log::Log4perl::caller_depth--;
}
$Log::Log4perl::LOGDIE_MESSAGE_ON_STDERR ?
@@ -993,16 +982,16 @@
##################################################
my $self = shift;
- my $msg = $self->warning_render(@_);
local $Carp::CarpLevel = $Carp::CarpLevel + 1;
+ my $msg = $self->warning_render(@_);
- my $message = Carp::longmess($msg);
- local $Log::Log4perl::caller_depth =
- $Log::Log4perl::caller_depth + 1;
if ($self->is_fatal()) {
+ my $message = Carp::longmess($msg);
+ $Log::Log4perl::caller_depth++;
foreach (split(/\n/, $message)) {
$self->fatal("$_\n");
}
+ $Log::Log4perl::caller_depth--;
}
$Log::Log4perl::LOGDIE_MESSAGE_ON_STDERR ?
