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 :)

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

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

On 2014-10-11 05:41, 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 :)

I think it's unacceptable that the documentation of "defaultLogFunction" and "trace", "info" and so on is merged. Same thing with "memLogFunctions". Do these even need to be exposed?

-- 
/Jacob Carlborg

On Saturday, 11 October 2014 at 10:48:00 UTC, Jacob Carlborg wrote:
> On 2014-10-11 05:41, 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 :)
>
> I think it's unacceptable that the documentation of "defaultLogFunction" and "trace", "info" and so on is merged. Same thing with "memLogFunctions". Do these even need to be exposed?

this has been used make user defined LogLevel log functions, like

trace1(...), trace2(...)

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

On 09/28/2014 02:24 PM, Dicebot wrote:
> Important changes since last review:
> - new approach for compile-time log level filtering

What's new here? It still relies on version identifiers to do so.
As I said in some earlier review, I think it's a bad idea for a library to rely on version identifiers that are defined in client code.
I will only work for templated code and makes it much harder for build tools. In a way it's the equivalent of
    #define LOG_LEVEL 2
    #include <logger.h>

I even proposed an alternative that uses type tags instead.
http://dpaste.dzfl.pl/95fb6a4e086d

> Usual process : 2 weeks for checking out if there are any critical
> issues that are likely to prevent successful voting, write a comment if
> you need more time for review, focus on API issues.

- Documentation is out of sync.
- ArrayLogger seems to do about the same as MultiLogger
- Why do loggers have to be classes?
  - Define a concept (isLogger template) with the requirements.
  - Add a Logger interface and a loggerObject to wrap structures when polymorphic loggers are needed.

On Saturday, 11 October 2014 at 13:16:18 UTC, Martin Nowak wrote:
> On 09/28/2014 02:24 PM, Dicebot wrote:
>> Important changes since last review:
>> - new approach for compile-time log level filtering
>
> What's new here? It still relies on version identifiers to do so.
> As I said in some earlier review, I think it's a bad idea for a library to rely on version identifiers that are defined in client code.
> I will only work for templated code and makes it much harder for build tools. In a way it's the equivalent of
>     #define LOG_LEVEL 2
>     #include <logger.h>

All that code is contained in 30 line template, That is by far the best working option anybody could come up with

>
> I even proposed an alternative that uses type tags instead.
> http://dpaste.dzfl.pl/95fb6a4e086d

And I showed that it did not work.

> - Documentation is out of sync.
gh-page is yes, give me 15min

> - ArrayLogger seems to do about the same as MultiLogger
have you read my reply to Jacob
> - Why do loggers have to be classes?
As answered multiply times before, to build log hierarchies.

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 :)

MultiLogger got a new simpler impl.

On 2014-10-11 14:08, Robert burner Schadek wrote:

> this has been used make user defined LogLevel log functions, like
>
> trace1(...), trace2(...)

I still don't like the merge of the documentation.

-- 
/Jacob Carlborg

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 :)

I don't think I will use it for the reasons I stated in the previous thread, so no point in saying anything bad.