AMDG Gennadiy Rozental wrote:
The formal review of Logging library, proposed by John Torjo, begins today and will run till Feb 13:
Here's part 1 of my review from reading the docs only. More to follow after I look through the implementation. in main_intro.html there is a line that says Click to see the code which sounds like it ought to be a link but isn't. scenarios_code.html#scenarios_code_mom: I don't think that write is the correct name: g_l()->writer().write("%time%($hh:$mm.$ss.$mili) [%idx%] |\n", "cout file(out.txt) debug"); This doesn't write anything. It seems like this should be called something more like set_format_and_destination() I don't like the idea of two stage initialzation implied by g_l()->mark_as_initialized(); Is there any way that the initialization can be done where the log is defined? Concepts as Namespaces: I strongly dislike this way of expressing it. What you mean is that you put implementations of that concept in a specific namespace. The concept, per se, has no relation to the namespace. This description is very misleading, IMO. What does the suffix ts stand for? Oh. I see. Thread Safety. Could this be spelled out completely? It's not going to appear all over the code so there is no reason to make the name as short as possible. boost::logging::formatter::idx: Please! don't abbreviate this. "index" is only *two* characters longer. Workflow.html: You'll use this when you want a optimize string class. Or, when using tags s/optimzed/optimized/ s/a/an/ Is there a way to have destination specific formatters? Throughout the docs some #include's directives are links to the corresponding headers and other are not. I find the use of preprocessor directives vs. templates for customization not very intuitive. For example, the options controlling caching are macros. I'm not convinced that this should be a global setting. The fast and slow compile time options make sense I think because they are transparent to the user. (BTW, would I need to #ifdef out the #include <boost/logging/format.hpp> to have the compile times improve?) AT the bottom of macros.html: #define BOOST_LOG_TSS_USE_CUSTOM = my_thread_specific_ptr The "=" is wrong I think. If BOOST_LOG_NO_TSS is defined is it an error to use the thread-specific-storage filters? filter::debug_enabled/release_enabled: How is debug/release determined. NDEBUG? in namespaceboost_1_1logging_1_1manipulator.html there is a reference to destination::shared_memory - writes into shared memory (using boost::shmem::named_shared_object) is this obsolete? namespaceboost_1_1logging_1_1manipulator.html#manipulator_share_data: It doesn't look like the example here will compile. Second, is there any particular reason not to use shared_ptr directly? structboost_1_1logging_1_1manipulator_1_1is__generic.html: I'm don't understand the reference to the template operator=. namespaceboost_1_1logging_1_1formatter_1_1convert.html "explain that you can extend the following - since they're namespaces!!! so that you can "inject" your own write function in the convert_format::prepend/or whatever namespace, and then it'll be automatically used!" I'm almost certain that this is wrong. In order to be found by a template the overloads need to be declared by the point where the template is defined, although if I remember correctly not all compilers implement this correctly. If you're going to rely on overloading for customization do it via ADL. Concepts: For the following concepts I either couldn't figure them out or had to hunt all over the documentation. This information should be specified in the Concepts section Filter: There is some way to get a boolean out of a filter. All the details are specified by the logging macro. Level: I can't figure out from the documentation what I would need to do to model this concept. Writer: Seems to be a specialization of a Unary Function Object? Scenario: This doesn't seem like a concept. I'm a little confused now. In addition I don't understand why ts and usage are disjoint. There seems to be significant overlap between the two. Is there any way that they could be merged? Gather: What exactly are the requirements? The docs say "a function that will gather the data - called .out()" What are the requirements on the return type? Further in scenarios_code.html#scenarios_code_mon there is the definition void *out(const char* msg) { m_msg = msg; return this; } Is out() called implicitly or not? Why is "this" returned as a void*? Confused... Tag: The only requirement on a tag is that it must be CopyConstructable (Is it Assignable, too?). tag::holder is a effectively a fusion::set of tags. It's not clear from either the tag or holder documentation how to extract tags in a formatter. One final comment about the Concepts: Manipulators: I think this framework is overly complex. It would be better to make manipulators Equality Comparable Unary Function Objects. In the case of destinations the return type is ignored, for for formatters, the result type should be the same as the argument type. For both the argument type is determined from the logger. You can use type erasure to allow them to be treated polymorphically at runtime. In Christ, Steven Watanabe