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.
