On Saturday, 11 October 2014 at 04:31:17 UTC, Jakob Ovrum wrote:
On Saturday, 11 October 2014 at 03:41:08 UTC, Dicebot wrote:
I don't see critical objections so far and this will move to
voting stage this weekend. Please hurry up if you want to say
something bad :)
Attributes need to be applied thoroughly. Even if most uses
will be through the base class `Logger`, it's still useful to
have stronger guarantees through a derived class reference.
This is particularly important because it's an important design
decision to choose which attributes to apply to `Logger`'s
methods.
@trusted is used everywhere instead of properly using @safe and
minimized @trusted. I think this is the third time I point this
out...
The multiloggers are a complete mess. There's both
`ArrayLogger` and `MultiLogger`, and while `ArrayLogger` has
simple O(n) operations, `MultiLogger` is a disaster: insertion
iterates all elements twice and sorts the entire collection on
every call, and removal iterates all elements once, then does
binary search twice. Once using `SortedRange`'s search, and
once using its own binary search algorithm. It also contains
debug code that writes to stdout. Neither type adheres to the
Phobos container concept, instead the underlying array is
exposed as a public, undocumented field. `string` is used
instead of `const(char)[]` for search and removal operations.
The latest std.container.Array broke the code anyway, so it is
due for a rewrite anyway.
The implementation of `Logger` has several performance
problems. `Logger` provides default behaviour that allocates GC
memory multiple times for even the simplest log messages
through the `Appender`. I don't think this behaviour should be
encouraged by putting it in the root logger class, and besides,
it can be made much more intelligent than just using a new
appender for each message.
Well, to have ultra simple thread-safe sub classing (which is an
important part of the design), this was the price. This being
said. Doing it nogc yourself if you know the output is very easy
as shown in FileLogger.
Another issue is that the way it's written currently,
`writeLogPart` is called a lot more often than necessary,
without any opportunity for optimization within
`formattedWrite`, thus `FileLogger` is doomed to write to the
underlying file character-by-character in easily reproducible
circumstances (e.g. log a range of characters); this issue
probably doesn't affect the API though.
Again, by design. To allow user created structured logging, this
is necessary.
`Logger` has a bunch of public and documented `*Impl`
functions...
see my other post
Some other line comments I posted a while ago have not been
addressed.
I will recheck github
