Review: std.logger

[Robert burner Schadek via Digitalmars-d]

On Thursday, 24 July 2014 at 22:27:34 UTC, Jakob Ovrum wrote:
> 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.

That is music in my ears...

>
> 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.

I do this lazily in a function, because having it global froze 
std.concurrency and std.process unittest. I couldn't figure out 
why.
As said earlier, I think GC and Logger is a none issue. I mean 
how often has anyone seen a Logger created in a loop over and 
over again.
nothrow will be hard as std.logger uses format, same for nogc

>
> 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.

So you're thinking of a stack array?

>
> 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.

What about the log functions and there implementation as well as 
the Logger specific LogLevel and name?
>
> I much prefer overloading over the mess of suffixes for the 
> same reasons Andrei mentioned.

I'm working on that.

>
> 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.

yeah, apart from StdIOLogger everything is thread unsafe. I like 
the template base class LockingLogger idea. I will see what I can 
do.

>
> 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.
+1