On 10/27/14, 1:49 PM, Dicebot via Digitalmars-d wrote:
> On Monday, 27 October 2014 at 20:42:10 UTC, Martin Nowak wrote:
>> Say I want to add tracing/logging to
>> [`parseJson`](http://dlang.org/library/std/json/parseJSON.html) or
>> [`findRoot`](http://dlang.org/phobos/std_numeric.html#.findRoot)
>
> This is exactly what is wrong :) Using std.logger inside Phobos itself is a big no. Actually even
> finding yourself in position where you may want to do it indicates some Phobos design issue.

Why?  I agree that large parts of phobos have little need for logging, but I question the general statement.  What's so special about phobos that suggests that no part of it should ever log?  How is it different from many other third party libraries that might be similar in nature?

Consider the std.net.curl.  I find it entirely reasonable to consider that it would have logging added to it.  It could be argued that that part of phobos could/should be removed, but ignore that for now and pretend its a full re-implementation of an http client instead if that helps.

Consider the scheduler work that Sean is doing.  I'll bet he's got printf's in there at some strategic points for debugging right now.  Most of those I can easily see translating into trace level logging.

The same holds for any higher level tasks that have any sort of complexities and need a way for developers to witness how those subsystems are executing after the fact.

My key point, phobos isn't and shouldn't be considered special here.  If it shouldn't be doing logging, then why and why doesn't that same logic apply to almost every other library that developers are going to use?

I suspect there's some philosophical differences at play.

My 2 cents,
Brad

On 10/27/2014 09:49 PM, Dicebot wrote:
> This is exactly what is wrong :) Using std.logger inside Phobos itself
> is a big no. Actually even finding yourself in position where you may
> want to do it indicates some Phobos design issue.

Well the same problem also applies to any other library that I obtain as binary release.

On 10/27/2014 01:36 PM, Robert burner Schadek wrote:
> Well, as far as I can see his argument was based on old code that has
> long been rewritten and he hasn't answered since I pointed that out.

How do come to that insight?

On 10/27/2014 09:28 AM, Robert burner Schadek wrote:
>
> If it where done this way, yes of course you're right. But it is not,
> please take a look a the source first.

I'm looking at https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d and this is exactly how this works.

Even if we make isLoggingActive a template, the problem persists.

cat > lib.d << CODE
version (StdLoggerDisableLogging)
    enum isLoggingActive() = false;
else
    enum isLoggingActive() = true;

void doSome()
{
    import std.stdio;
    writeln("loggingLib: ", isLoggingActive!());
}
CODE

cat > main.d << CODE
import lib, std.stdio;

void main()
{
    writeln("loggingMain: ", isLoggingActive!());
    doSome();
}
CODE

dmd -version=StdLoggerDisableLogging -lib lib.d
dmd main lib.a
./main
----
logginMain: true
logginLib: false
----
dmd -lib lib.d
dmd -version=StdLoggerDisableLogging main lib.a
./main
----
logginMain: false
logginLib: true
----

On Monday, 27 October 2014 at 20:49:35 UTC, Dicebot wrote:
> On Monday, 27 October 2014 at 20:42:10 UTC, Martin Nowak wrote:
>> Say I want to add tracing/logging to [`parseJson`](http://dlang.org/library/std/json/parseJSON.html) or [`findRoot`](http://dlang.org/phobos/std_numeric.html#.findRoot)
>
> This is exactly what is wrong :) Using std.logger inside Phobos itself is a big no. Actually even finding yourself in position where you may want to do it indicates some Phobos design issue.

Yes and No. His Logger can came from the user aka. outside of phobos. That might just be valid. But the problem with the design is that he needs to accept every possible Logger. And that either means template of abstract class Logger.

The problem with a template is:

``` library code that is given in binary form
auto fun(Logger l) {
   return parseJson(getData(), l);
}
```

their is no choice but to pass a class. Meaning you have to wrap the struct Logger with a class proxy. And this will properly develop into a common theme.

Allowing struct has one design problem IMO:
Either we force to callee to accept Logger as template or
we force the caller to wrap his Logger struct with Logger proxy classes.
This is because an abstract class is the lowest common denominator in this case.

Anyway, I'm pretty sure that Martin and I will never see eye to eye in this discussion. IMO disabling a single Logger through its LogLevel at compile (plus all the extra litter and possible needed wrapping) is no better than creating NullLogger. He thinks the opposite.

The problem for me now is, if I add a struct UFCS overload Martin will be happy but somebody else will stream WAT "The struct stuff must go"

So please everybody reading this, please give a comment about this.

On Monday, 27 October 2014 at 22:27:12 UTC, Martin Nowak wrote:
> On 10/27/2014 09:28 AM, Robert burner Schadek wrote:
>>
>> If it where done this way, yes of course you're right. But it is not,
>> please take a look a the source first.
>
> I'm looking at https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d and this is exactly how this works.
>

take a look at

https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L190
and
https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L579

isLoggingActiveAt is instantiated at CT of the log function and the version statement inside the first link is evaluated at that moment.

Disabling a version at CT of the lib has no consequence to compile units that are not compiled with that version statement.

I tested it at https://github.com/burner/logger/blob/master/Makefile#L23
run "make info"

On Monday, 27 October 2014 at 22:20:04 UTC, Martin Nowak wrote:
> On 10/27/2014 01:36 PM, Robert burner Schadek wrote:
>> Well, as far as I can see his argument was based on old code that has
>> long been rewritten and he hasn't answered since I pointed that out.
>
> How do come to that insight?

because the code you show as an example:

"cat > lib.d << CODE
version (StdLoggerDisableLogging)
    enum isLoggingActive() = false;
else
    enum isLoggingActive() = true;

void doSome()
{
    import std.stdio;
    writeln("loggingLib: ", isLoggingActive!());
}
CODE"

is different from the code that has been in the PR for quite some time. And the code you show does exactly what you say and the current code does something different.

On 10/28/2014 12:58 AM, Robert burner Schadek wrote:
>
> Disabling a version at CT of the lib has no consequence to compile units
> that are not compiled with that version statement.

Yes setting a version in my app has no effect on the library, that's the problem because I cannot disable logging in a library.
So I'll always have to pay some runtime overhead even though I don't want to use logging.

On 10/28/2014 01:01 AM, Robert burner Schadek wrote:
> is different from the code that has been in the PR for quite some time.
> And the code you show does exactly what you say and the current code
> does something different.

No it behaves the same.

isLoggingActive is a template in phobos

doSome is a function in a lib that performs logging and instantiates isLoggingActive

main is a function in the app that performs logging and instantiates isLoggingActive and also calls doSome

Now which of those functions actually logs depends on the compilation settings of the library, the compilation settings of the app and the logger that's being used.

On 10/25/14 9:43 AM, Dicebot wrote:
> Jut for the reference, my position on current state of things as review
> manager is this:
>
> I agree with some of mentioned concerns and would like to see those
> fixed. However, I don't think any of those are truly critical and this
> proposal has been hanging there for just too long. In my opinion it will
> be more efficient to resolve any remainining issues in follow-up pull
> requests on case by case basis then forcing Robert to do it himself -
> there will be some time left before next release to break a thing or two
> before public statement is made.
>
> Because of that I am going to start voting despite some arguments being
> still in process. I hope that won't cause any tension.

Being able to select maximum logging level statically at client application level is a deal maker/breaker for me. The mechanics aren't important but it's likely they will affect the API. So I think that needs to be resolved now, not in a future pull request.

Andrei