On 7/11/14, 7:36 AM, 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.
Preface: (1) We already use the std.logger candidate at Facebook; (2) I
really want to get this in, it's been past time already.
Overall: I think std.logger has a passable overall design having
simplicity and no-nonsense as compelling advantages. However, there's
some serious tactical work needed.
0. There's no way to set the minimal logging level statically, except
for an on/off switch. There must be a way to define e.g.
-version=logLevel=trace that sets the minimum logging level actually
performed. Any logging below that level is a no-op. Which segues into
the next problem:
1. There's a bunch of code still generated even with logging statically
disabled (per http://d.godbolt.org). I could not find a way to
completely eliminate generated code. Unused lazy parameters MUST
generate no code at the front end level. This is a performance bug in
the D front end that blocks acceptance of this proposal.
2. The three one-letter suffixes don't seem to scale well. They optimize
for brevity but at the cost of clarity. Also they establish a precedent
- are we sure we recommend future D code in the standard library and
beyond to mangle names using one-letter conventions? I think we must
find a way to solve things via overloads.
Currently we have: log, logc, logf, logl, logcf, loglf, loglc, loglcf.
One would almost expect logwtf to be somewhere in there.
I think an overloading-based solution would be a better choice here.
First, I think the logging level must guide all overloads. True, we'd
sometimes like to log the logging level itself, but that's easily done
by using a formatting function (as shown below). So:
log(A...)(lazy A stuff); // just log whatevs
log(A...)(LogLevel lvl, lazy A stuff); // log at specified level
log(A...)(bool c, LogLevel lvl, lazy A stuff); // conditionally log at
specified level
Then three more logf with similar signatures, using the precedent set by
write/writef. And we're done.
To log the logging level itself, just use logf(someLevel, "%s",
someLevel) and be done with it.
There's no conditional logging without specifying a level, which should
be fine seeing as conditional logging is not that frequent to start with.
There should be some shortcuts for logging levels such that one can
write log(info, "crap") instead of log(LogLevel.info, "crap").
3. I'm not sure I like the design using defaultLogger in conjunction
with free functions using it. It clearly makes for comfortable casual
logging by just calling log(whatevs) and it uses a precedent set by
stdout. But I wonder if it would be cleaner to just give it a shorter
name "log" and then have it have a "write" method:
log("crap");
-> becomes ->
log.write("crap");
Also there'd be log.writef("%s", "crap") etc.
4. Conditional logging must be justified. In my mind the justification
is that statically setting the log level makes the code disappear
without the condition being ever evaluated, but the current design
doesn't allow setting the log level!
5. There was some nice stuff in the previous std.logger work by me and
later Jose (I think), which allowed logging every n times/milliseconds
so as to allow nice throttling. That's nice to omit/defer for
simplification purposes, but I noticed that log noise is a serious matter.
6. The current backend design requires use of classes and references,
i.e. garbage collection. Amid the current tendency to make std work
without requiring GC, I think a design based on RefCounted would be
recommended here.
7. The current backend design fills a struct with data then passes it to
the implementation. But if the implementation doesn't use e.g. the
timestamp then that work has been wasted. Maybe offer the fields as
properties instead, with caching upon first use?
8. Documentation needs work as it has disfluencies and typos.
9. I've also posted a bunch of comments to the code at
https://github.com/D-Programming-Language/phobos/pull/1500/files
Andrei
