==============================
David Nadlinger
==============================

"I agree. For this reason, I also vote for "no" (1 as well as 2),
as the current conditional logging support doubles the size of
the API for shaving a grand total of 3 characters off the
invocation in a rather infrequent use case."

Addressed.


"Wow, upon further code review I discovered that Logger actually
overrides opCmp/opEquals to be based on the name (?!). This leads
to the following gem: < .. >"

Fixed.

==============================
Martin Nowak
==============================

"Get rid of the 8 different suffixes.
I only see the need for log and logf, why is the rest needed?"

Addressed.

"Support duck-typing for the log functions.
Logger should be a concept and log functions should be free-standing
UFCS functions that take any `isLogger!T`.
To support a global `defaultLog` variable, you could add a Logger
interface and a loggerObject shim. See
http://dlang.org/phobos/std_range.html#inputRangeObject for this a pattern."

Neither seem to be addressed nor countered.

I have likely missed several points but overall it seem pretty clear to me that all requests / concerns have not been addressed and this proposal is not yet ready for another round of review.

Also x-post from GitHub PR of my personal nitpick:

"... have noticed that all logging functions have file/line/function data as template parameters. This will create insane symbol bloat. While I can understand desire to use some nicer variadic argument API providing at least one log function that it simplistic but moves all compile-time data to run-time default argument is absolutely necessary for me to consider this viable."

On 8/30/14, 5:18 AM, Dicebot wrote:
> I have likely missed several points but overall it seem pretty clear to
> me that all requests / concerns have not been addressed and this
> proposal is not yet ready for another round of review.

How do the remaining issues break down into QoI that can be deferred to a future release? -- Andrei

On Saturday, 30 August 2014 at 02:11:49 UTC, Dicebot wrote:

> "API must specify a strong stance on threading, whatever the
> form it takes"
>
> Does not seem to be addressed at all. At least I see no mentions of it in core.d documentation and logger instance itself is plain __gshared thing.

I'm not a "voter" as far as I know, but I suggest that Logger is formally documented to be not-thread-safe, and that it is the responsibility of subclasses to provide thread safety.  Actual Logger's can do so easily by overloading beginLogMsg/logMsgPart/finishLogMsg and not using a shared class variable between calls.  Loggers could also remain thread-local but overload writeLogMsg to dispatch the result to thread-safe I/O.  The latter is the path I have chosen currently for log4d (https://github.com/klamonte/log4d).  (That may change in the future if there is memory pressure from too many thread-local logger instances.)

> "We need to hammer out how this will work inside libraries.  If my app is
> using multiple libraries I need to know I have full control of how they
> log and where (), and if I write libraries I need to include logging that
> will not affect performance or added dependencies.

This is a task for a fuller backend, not the std.logger frontend.  std.logger should be "mechanism", interfacing multiple libraries together requires "policy".

> "Logger should include a shared Logger, or include it in the interface for
> outside libraries to handle the implementation.  There will be libraries
> that thread internally and will need to support shared logging."
>
> Is not addressed.

stdlog is global and the default implementation is thread-safe by way of FileLogger mutex-locking its writes.  It should either be documented that stdlog is always global and it is up to subclasses or logging backends to make stdlog thread-safe, or Logger should be made thread-safe by default by eliminating it's header and msgAppender fields.  I'm not sure either way which is better.

I don't like the idea that libraries A, B, and C will just blindly use stdlog and hope for the best.  If they are "logging" (and not "tracing", for which I hope D evolves a @trace attribute) then they should be logging by category/name, which is a job for something like log4d.  But Logger's don't have names.

Perhaps the better solution would be a convention for libraries to always use a "static Logger Logger.getLogger(string category)" function, for which the default simply ignores category and returns stdlog.  "Logger.setGetLoggerFunction(...)" could be provided for backends to change this behavior on application startup, assuming they can guarantee that they can call this function before various libraries have gotten references to stdlog.

On Saturday, 30 August 2014 at 09:54:36 UTC, Andrei Alexandrescu wrote:
> On 8/30/14, 5:18 AM, Dicebot wrote:
>> I have likely missed several points but overall it seem pretty clear to
>> me that all requests / concerns have not been addressed and this
>> proposal is not yet ready for another round of review.
>
> How do the remaining issues break down into QoI that can be deferred to a future release? -- Andrei

What is QoI? I am not familiar with this abbreviation

On Saturday, 30 August 2014 at 02:11:49 UTC, Dicebot wrote:
> ====================================
> Jakob Ovrum
> ====================================
>
> "The *API* must support minimal dynamic memory allocation for
> the normal execution path. However, theoretical *implementation*
> changes that reduce memory allocation are not a deal-breaker."
>
> This seems to be addressed but though it is desired to verify it via @nogc unittest block which uses stub no-op logger (to verify that nothing in between allocates). One place were GC allocations is unavoidable is core.d:1628 - it will always create FileLogger first and allow assigning custom one later. Is there any way to circumvent it?

Yes - split the `stdlog` property into a getter and a setter. Then if the setter is called first (with a non-null reference), the getter never gets to allocate the default instance. I pointed this out in a line comment before, but I guess it disappeared due to the name change...

Also, as I said in the line comment, it doesn't need to GC-allocate, it can allocate in global memory or TLS using emplace (which of them is appropriate depends on the threading model, I guess...). If Andrei's reference-counting suggestion is implemented, then depending on the details, it's possible that the default logger could be allocated on the C heap instead of the GC heap as well, managed by RC.

On Sat, Aug 30, 2014 at 01:08:05PM +0000, Dicebot via Digitalmars-d wrote:
> On Saturday, 30 August 2014 at 09:54:36 UTC, Andrei Alexandrescu wrote:
> >On 8/30/14, 5:18 AM, Dicebot wrote:
> >>I have likely missed several points but overall it seem pretty clear to me that all requests / concerns have not been addressed and this proposal is not yet ready for another round of review.
> >
> >How do the remaining issues break down into QoI that can be deferred to a future release? -- Andrei
> 
> What is QoI? I am not familiar with this abbreviation

Quality of Implementation.


T

-- 
The peace of mind---from knowing that viruses which exploit Microsoft system vulnerabilities cannot touch Linux---is priceless. -- Frustrated system administrator.

On Saturday, 30 August 2014 at 13:35:57 UTC, H. S. Teoh via Digitalmars-d wrote:
>> >How do the remaining issues break down into QoI that can be deferred
>> >to a future release? -- Andrei
>> 
>> What is QoI? I am not familiar with this abbreviation
>
> Quality of Implementation.

And how can one break issues into quality of implementation? :O I have literally no idea what Andrei means.

On Sat, Aug 30, 2014 at 01:37:15PM +0000, Dicebot via Digitalmars-d wrote:
> On Saturday, 30 August 2014 at 13:35:57 UTC, H. S. Teoh via Digitalmars-d wrote:
> >>>How do the remaining issues break down into QoI that can be >deferred to a future release? -- Andrei
> >>
> >>What is QoI? I am not familiar with this abbreviation
> >
> >Quality of Implementation.
> 
> And how can one break issues into quality of implementation? :O I have literally no idea what Andrei means.

I'm guessing he means, break them down into must-solve-before-merge issues and stuff-to-be-fixed-later issues.


T

-- 
Acid falls with the rain; with love comes the pain.

On Saturday, 30 August 2014 at 13:37:17 UTC, Dicebot wrote:
> On Saturday, 30 August 2014 at 13:35:57 UTC, H. S. Teoh via Digitalmars-d wrote:
>>> >How do the remaining issues break down into QoI that can be deferred
>>> >to a future release? -- Andrei
>>> 
>>> What is QoI? I am not familiar with this abbreviation
>>
>> Quality of Implementation.
>
> And how can one break issues into quality of implementation? :O I have literally no idea what Andrei means.

Implementation issues can be fixed later as long as the API allows for it, so we should focus on the latter.