Review: std.logger

Sönke Ludwig via Digitalmars-d digitalmars-d at puremagic.com
Sat Jul 12 09:13:28 PDT 2014


Overall looks good to me. Some points that haven't been mentioned so far 
in this review round:

  - Using a class with static members doesn't seem to be very idiomatic. 
It seems like the three member properties can simply be made global and 
everything should be fine - the "LogManager." prefix doesn't really add 
information. This has been mentioned in the last review round and it's 
not a very important point in this particular instance, but we should 
really make a decision here that will also decide how future modules go 
about this.

  - Personally, I really find additional verbose log levels useful. 
Currently there is only "trace". Previous proposals had a generic 
"verbose(N)" set of levels, but I think it's important for the proper 
interaction of multiple libraries to define a semantic set of verbose 
levels. A proposal, which has worked very well me (from low to high):

   trace:
      as now, used for low level tracing of program flow
   debugv:
     verbose debug messages, may flood the log
   debug:
     normal, low frequency debug messages, useful for the developer
   diagnostic:
     diagnostic output also potentially useful for the user
     this is what you typically would get with the -v command line switch

  - There is a "formatString" string mixin that includes a lot of 
if-else cases that seem to do exactly what formattedWrite would do 
anyway, is there something that it actually does in addition to that? 
Also, why is it a string mixin in contrast to a simple (template) 
function? It may be a matter of taste (but also of compile time/memory), 
but I'd almost always prefer a non-string-mixin solution.

  - Even if it may be more typing (or maybe not?), actually writing out 
the different signatures for each log level and the c/f suffixes would 
be very advantageous for the documentation and for code completion. It 
would also make the EBNF unnecessary, which I agree with Johannes looks 
a little scary. All of the functions could be based on the generic 
logl/loglf functions, so that there wouldn't be much redundancy.

  - I'm still really not sure if the "c" versions of the log functions 
pull their own weight. They seem to be in line with trivial convenience 
functions, which are generally discouraged in Phobos (with the same 
issues, such as additional cognitive load and bigger API size).

  - The functions error(), info(), fatal(), etc. don't follow the usual 
rule for functions to start with a verb. The question is if saving three 
characters over logError() is worth making the code more ambiguous for 
the outside reader (e.g. "does error() throw an exception? or set some 
internal error state?" "does fatal() terminate the process?")



More information about the Digitalmars-d mailing list