Review: std.logger

[Jakob Ovrum via Digitalmars-d]

On Friday, 11 July 2014 at 14:36:34 UTC, Dicebot wrote:
> Round of a formal review before proceeding to voting. Subject 
> for Phobos inclusion : http://wiki.dlang.org/Review/std.logger 
> authored by Robert Schadek.

First I want to say that I want this to be *the* logging library, 
just as I always want a D library to be the best library in the 
world at what it does, as I believe D allows us to do that. I'm 
confident std.logger can become that, so thank you Robert for 
your work.

As for using class-based polymorphism; this doesn't have to mean 
GC memory. The only part of the library code that should have a 
need to allocate a logger class is the lazy initialization in 
`defaultLogger`. If we change this to allocate in global memory, 
then it's entirely up to the user how to allocate logger 
instances. Giving up classes here because of GC means giving up 
classes in no-GC code entirely, which I think is a serious 
overreaction. I think there are reasons to question the 
class-based polymorphism, on grounds such as - do we require that 
`writeLogMsg` is @nogc so we can log in @nogc code? What about 
nothrow? When it comes to performance and the indirect call 
involved, I don't think there's a way around that.

When it comes to GC memory, MultiLogger using an associative 
array is a blocker. However, I don't think it can be elegantly 
fixed as we don't have pluggable allocators for any of our 
standard containers yet. Maybe using an array (perhaps sorted) is 
an acceptable compromise measure if @nogc is deemed a priority.

One thing that really bugs me though, is that `Logger` is an 
abstract class and not an interface. An abstract class should 
only be needed when it has both pure virtual functions as well as 
default functionality that can be conditionally overridden, so 
`Logger` should not be a candidate. It should be rewritten in 
terms of an interface, which enables users to implement logger 
functionality in any of their classes, instead of having to 
dedicate a new class type just to override one virtual function.

I much prefer overloading over the mess of suffixes for the same 
reasons Andrei mentioned.

The library's stance on thread safety needs to be clearly 
defined. Currently, `defaultLogger` is process-wide, which can 
only mean logger instances must be thread-safe. Yet there is no 
mention in the documentation that loggers must be thread-safe, 
and indeed I think most of the default-provided loggers are 
written with no concern for thread safety.

I suggest one of two approaches: 1) make `defaultLogger` TLS and 
rework the documentation so it's clear that each thread must 
manage their own logger, or 2) make it clear that `defaultLogger` 
must be thread-safe, and take extreme care in the 
default-provided loggers that they are indeed thread-safe. Maybe 
a templated base logger class `LockingLogger` or something could 
help here.

The documentation needs a lot of work, but I think anyone can 
help with that. I intend to file a pull request to Robert's fork 
with fixes I could spot; it seems more efficient for both of us 
than posting an endless stream of line comments.