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
