Post by Dicebot via Digitalmars-d
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
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
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
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.
`Logger` has a bunch of public and documented `*Impl` functions...
Some other line comments I posted a while ago have not been