Skip Menu |
 

This queue is for tickets about the Log-Log4perl CPAN distribution.

Report information
The Basics
Id: 38356
Status: resolved
Priority: 0/
Queue: Log-Log4perl

People
Owner: Nobody in particular
Requestors: rabbit [...] rabbit.us
Cc:
AdminCc:

Bug Information
Severity: Unimportant
Broken in: 1.17
Fixed in: (no value)



Subject: Weirdness in callerline(), and_warn(), and_die()
Download (untitled) / with headers
text/plain 1.2k
Hi, I am in the process of writing an elaborate layer on top of Log4Perl, and I came across an inconsistency in Log::Log4perl::Logger::callerline(). I was planning to use it the same way you are, i.e. to construct messages to feed to die() and warn(), since I invoke them in a different manner under different circumstances. However it does not work for me because instead of using the global $caller_depth, this subroutine resorts to arcane call-stack scanning in order to determine the caller. Of course the caller always ends up being the first package outside of the log4perl namespace. Is there a reason for this, or is this simply an oversight and $caller_depth would be more appropriate there? If yes I am willing to submit you a properly tested patch. 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? And last but not least - this is a great module, great concept, great integration with the perl paradigm. However NOWHERE in the documentation, tutorials, faqs and pretty much any official text, NOWHERE does it say that a `category` and a `logger` are in fact ONE AND THE SAME THING. It took me almost a week of searching testing and reading code to get to this conclusion. Shame! :)
Download (untitled) / with headers
text/plain 1.4k
On Sat Aug 09 09:51:20 2008, rabbit@rabbit.us wrote: Show quoted text
> I am in the process of writing an elaborate layer on top of Log4Perl,
Excellent :). 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. 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. Show quoted text
> And last but not least - this is a great module, great concept, great > integration with the perl paradigm. However NOWHERE in the > documentation, tutorials, faqs and pretty much any official text, > NOWHERE does it say that a `category` and a `logger` are in fact ONE AND > THE SAME THING.
I couldn't find it either :). I could have sworn this is mentioned somewhere, but it's probably buried deeply and deserves a more prominent place. I've added it first thing under the "Category" section. Thanks for letting me know.
Subject: Re: [rt.cpan.org #38356] Weirdness in callerline(), and_warn(), and_die()
Date: Thu, 21 Aug 2008 03:32:31 +0200
To: bug-Log-Log4perl [...] rt.cpan.org
From: Peter Rabbitson <rabbit [...] rabbit.us>
Download (untitled) / with headers
text/plain 1.8k
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
Download l4p.diff
text/x-diff 5k
? 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 ?
Subject: Re: [rt.cpan.org #38356] Weirdness in callerline(), and_warn(), and_die()
Date: Wed, 10 Sep 2008 14:09:58 +0200
To: bug-Log-Log4perl [...] rt.cpan.org
From: Peter Rabbitson <rabbit [...] rabbit.us>
Download (untitled) / with headers
text/plain 242b
Hi Michael, Sorry for nagging, but I checked the CVS and the patch I sent a month ago does not seem to have been applied yet. Please let me know if there are any objections to the patch, or did it simply fall of the radar. Thanks! Peter
Download (untitled) / with headers
text/plain 158b
Hi Peter, sorry for the slow response, I've looked over the patch and it looks great! Applied to CVS, will be rolled out with 1.19. Thanks so much. -- Mike


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.