
Sorry for the long post in advance...
* What is your evaluation of the design?
In short, questionable. More elaborate notes follow: * To my mind, the named destination design approach is not intuitive. I would expect the writer to accept a stream (or a number thereof) as a destination, not a string names and their definitions. Such code is more difficult to modify - you can misspel the destination name or forget one in the list if names in destination::named and it'll only show in run time. It also complicates making the list of destinations configurable (e.g. loading logging settings of the user's app from a file). * The approach to adding formatters is not very intuitive. Seeing this piece of code: g_l()->writer().add_formatter( formatter::idx() ); g_l()->writer().add_formatter( formatter::append_newline() ); g_l()->writer().add_formatter( formatter::tag::file_line() ); g_l()->writer().add_formatter( formatter::tag::level() ); I cannot tell what the output will look like. A lambda-like syntax would be much better. * Lifetime of the objects provided by users, such as streams, should be controlled by the library. There should be no cases when you require the user to make sure the object exists long enough for the library. Use shared_ptr instead. The destination::stream is an example of such case. * Filtering looks way too minimalistic to me. As far as I can see, a filter is always static in the way it doesn't decide which particular log record passes and which does not. It just has a flag which tells what to do. Additionally, I see levels concept which I feel is very close to filters but has a different interface for some reason. You may argue that it's up to user to provide filtering logic, but the library provides no sufficient interface to implement a filter more complex than check a flag or a number thereof. * I don't see the reason of all these macros for declaring and defining logs, tags, etc. I would rather prefer to see functions or objects forward declarations instead. * I'm not sure I got it right with log records caching and mark_as_initialized function. Does it mean that until this function is called all log records are stored in memory and when it gets called all records are flushed to the writers? Is it possible then to alter the set of writers or destinations in run time after mark_as_initialized is called? Anyway, I don't like functions like this, they are always a source of mistakes. * I see that there is no easy way to use once-initialized logs in module A (exe or dll) in another module B. This would only be possible if linking these logs from A, thus making a dependancy between A and B, right? I have seen the dll_and_exe example and each module there has its own logging objects and its own code of their initialization. I don't like such code duplication. * The most critical note, from my point of view. I didn't find any trace of attributes or something like that. The library focuses on processing raw strings and there is no way to pass any additional data to the writers (I underline, the data as a typed object, not the formatted string). Tags look like an attempt to implement attributes support but I got the impression that they are only meant to rearrange data in the formatted string and are not capable to aid filtering and cannot be processed by writers as an independent piece of data. This is a major drawback at the library extensibility side. I remember, someone in the thread has also noted that the raw text logs make little use in the enterprise-scaled applications. I totally agree and add that even formatting the text (e.g. trying to put some layer of managing attributes on top of the existing raw text oriented solution) does not help much. I had a pleasure of analyzing gigabytes of logs to track down a rarely seen bug that one of our customers encountered and without a proper support for attributes and filtering based on them it is a nightmare. * The design is not OOP-friendly. I can't create a logger object that is specific for a request that my application is currently processing (or can I?). This might be very useful if I wanted to log the context of execution (the request parameters, again, as attributes or a similar feature) in each log record. One can try to emulate such feature with a more elaborate logging macro definitions but this is surely not the way to go. * The "optimize" namespace and "favor" stuff is a questionable. IMO, the code should not contain things like this. Why would I chose to use non-optimized features? Because optimized ones lack some features or don't work in some cases? Which are those then? What are the guidelines for their usage? But actually, I, as a user, don't need to know these details. The library should function correctly and effectively without involving the user into the optimization process. If there is a more optimal way to provide some functionality, the library should enable it itself, not involving the user. For example, std::distance just does the right thing in the most optimal manner transparently for the user.
* What is your evaluation of the implementation?
I didn't dig too deep into the code, but here are my 2 cents: * Maybe a compilable configuration should be provided to reduce user's code build times. This would be a better solution than to depend on a macro that has different default values in release and debug. * A better header segregation is needed. And it would be good to see in docs what I have to include to use each component of the library. * Line wrapping is needed. BTW, there's a requirement on this here: http://www.boost.org/more/lib_guide.htm#Design_and_Programming * I can see that filtering may unnecessarilly block threads. That could be fixed with read/write mutexes. * Use __LINE__ with care when compiling on MSVC 7.0+ with /ZI. You may run into problems when forming unique variable names. Use __COUNTER__ instead in such cases. * Don't count on that pthread_t is ostreamable. Some platforms define it as a fundamental type, some as a structure. It may well be a char* which will most likely cause crashes in your code. * Strictly speaking you are not guaranteed to have only a single thread before main. A thread may be spawned in a namespace scope object constructor. Either this case should be mentioned in the docs, or you should protect function-local statics against multithreading. Actually, you need that protection in either way because of cache synchronization issues. After being initialized in one thread the function-local static may look not initialized or partially initialized to another thread on another CPU.
* What is your evaluation of the documentation?
Needs a bit restructurization. I'd like to see it separated: a simple usage tutorial, advanced features description (logging in depth :)), extending the library section, concepts and rationale sections. It would also be good if it followed the common Boost docs style.
* What is your evaluation of the potential usefulness of the library?
I have a dual feeling about this. On one hand it provides a good set of features to implement logging in a relatively small and simple application. On the other hand, would I bother using it instead std::cout? Maybe.
* Did you try to use the library? With what compiler? Did you have any problems?
No, I did not compile anything.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
About 4 hours of reading docs, examples and the library code.
* Are you knowledgeable about the problem domain?
I believe, I am.
* Do you think the library should be accepted as a Boost library?
No, at least at its current shape. I expect something different and more elaborate from the logging library. I believe that logging is not only writing strings into a file but it is an event reporting system. The library should be flexible enough to be able to support statistics gathering and alarming. The library should provide a flexible filtering mechanism based on attributes of the logging records. I cannot see this in the library, along with other less fundamental features.