On 10/14/2013 04:44 PM, Sönke Ludwig wrote: > Am 14.10.2013 15:12, schrieb Robert Schadek: >> On 10/14/2013 02:39 PM, Sönke Ludwig wrote: >>> - The static methods in LogManager should be made global and the class >>> be removed. It's not for objects so it shouldn't be a class. >> LogManager also stores the global log level. Sure I can make another >> static global function storing this log level, but I would like to keep >> them together as they belong together IMO. > The same could be said about the global "log" functions, which are > tightly coupled to that state. I think this is already nicely grouped > together by the logger module itself, since there is not much else in it. > > Basically, I just wouldn't consider this style to be particularly > idiomatic D code, but of course that's just personal > perception/preference (there is also some precedence using "struct" > instead of "class" in Druntime). However, if it ends up like this in the > final version, it should get a "@disable this();" to prevent misuse. It is for ment for phobos not druntime. Anyway structs would mean all templates and people will scream template bloat. And this would break the design, which I find to be a valid use of classes and polymorphisms. The StdIOLogger can have its default constructor called IMO. > >>> - For me this logger is completely worthless without any debug log >>> levels. The last std.log entry had at least anonymous verbosity >>> levels, but I'd prefer something like I did in vibe.d [1], where >>> each level has a defined role. This should especially improve the >>> situation when multiple libraries are involved. >> Logger.log(LogLevel.(d|D)ebug, "Your message"); > That would be my idea. Having at least two (diagnostic output for the > user and debug output for the developer), but better all four debug > levels can be very useful, though. Maybe I miscommunicated what I want to show by that example. The (d|D) part is the rename to enum lower case. The debug log level is given through the LogLevel.Debug, which will be renamed to LogLevel.debug. I would call the developer the user of the logger. Maybe log messages can be communicated to the user of the applicaiton and the developer of the application through a MultiLogger class. >>> - Similarly, I've never felt the need for conditional logging, but >>> without it being lazy evaluated what's the use for it, actually? >> The conditional logging part is totally transparent. > But is there a compelling use case? It's transparent, but still > increases the size/complexity of the API, not much, but there should be > a reason for that IMO. I had the case that a custom class made problems in some special case. And IMO it was just prettier to have the condition if it should be printed passed as first argument rather than adding a if branch in my top level code.
log(cls.canBePrinted(), "%s", cls.toString()); is IMO prettier than if(cls.canBePrinted()) { log("%s", cls.toString()); } > >>> - I really think there should be shortcuts for the different log >>> levels. Typing out "LogLevel.xxx" each time looks tedious for >>> something that is used in so many places. >> One could argue that writting logger.logDebug("...") is more tedious >> than writing, >> logger.logLevel = LogLevel.xxx; >> logger.log("..."); >> logger.log("..."); >> ... >> >> This has been argued in the last logger discussion to some extend and it >> looked to me like this is the mostly preferred version. > The last discussion resulted (if I remember right) in something like > "log.info(...)". This would be fine, too, although I think just > "logInfo" is slightly preferable due to its length and the principle of > least surprise. At least jmdavis had a very strong argument against it. Something like function should be verbs ... Maybe something like logCritical, logInfo would be a compromise, but I think everybody here has a different idea of what is correct. >>> - There should be some kind of MultiLogger so that multiple log >>> destinations can be configured (e.g. console + file). Or, instead of >>> a single default logger, there could be a list of loggers. >> there is one default logger. I will create a MultiLogger, good point. >> I'm currently sure how to store the multi logger (LL or Array or ... ) > I'd say a dynamic array is fine because the list of loggers will rarely > change and this would be most efficient for iteration. Properly yes, at least will that be my first try. > >>> - On the same topic, if I'm right and the default logger is stored as >>> __gshared, it should be documented that Loggers need to be >>> thread-safe. >> It is not stored __gshared, but If, you're right. > So the defaultLogger is per-thread? That may result in unexpected log > output if you just do simple code like "spawn({ log("Hello, World!"); > });" and have a custom logger set up during application initialization. > Anyway, let's just say the threading behavior in general should be > clearly documented here as this is critical for both log users and > Logger implementors. Yes it is per thread. I concur that this needs to be documented well. >>> - GC allocations for each log message _must_ be avoided at all costs. >>> Using formattedWrite() instead of format() on either a temporary >>> buffer that is reused, or, better, on some kind of output range >>> interface of the Logger would solve this. >> This was proposed in the last thread. A fixed size buffer would scream >> bufferoverflow, a dynamic buffer not but both would raise the question >> of thread safety. > Something like a thread local buffer that grows when needed, or small > fixed buffer + a scoped heap allocation for large messages should be > fine. Using an output range interface could of course avoid buffering > each message altogether. That would just open up the question of a nice > API design for such. Maybe, but I bet somebody else will bring up Allocator and Ref Counting in that context. > > One last thing just occurred to me, in the default logger in vibe.d I've > made it so (thanks to Jordi Sayol for requesting this) that the "info" > level gets output to stdout and all other levels to stderr. Either that > or always outputting to stderr would be conforming to the idea of > stdout/stderr and would avoid that some library with logging calls > interferes with the output of an application (when piping process output > in a shell script). First, the default logger is assignable so maybe we should talk about the StdIOLogger which prints to StdIO or maybe StdErr. IMHO { I hate when that happens, I hate writing redirects to the cmd. } But this is properly a task issue. > > Thanks for bringing this forward! To whom is this directed?