On 11/6/05, Hartmut Kaiser <hartmut.kaiser@gmail.com> wrote: - What is your evaluation of the design? I think the design is overall quite good. I like the named logger concept and the run-time configurability of the system is a great strength. - What is your evaluation of the implementation? I have some questions and suggestions: The namespace has been changed from the original poposed versions from "log" to "logging" but the subdirectory names remain "log". This should be reconciled. boost/log/log.hpp: - Should BOOST_LOG_DEFINE_LEVEL use BOOST_STATIC_CONSTANT? - The "default_" level should be renamed "default_level". - enabled_logger::enabled_logger (logger&): initialize m_stream in the member initialization or the ctor body but not both - Remove old versions of macro definitions that have been rewritten (BOOST_IS_LOG_ENABLED, BOOST_SCOPED_LOGL). boost/log/log_impl.hpp: DEFAULT_INDEX should probably be lower-cased and possibly renamed. It should probably use BOOST_STATIC_CONSTANT. boost/log/log_manager.hpp: DEFAULT_CACHE_LIMIT: See concerns above. boost/log/functions.hpp: Macro guard looks like the victim of a Find/Replace gone awry: JT_BOOST_appender_funcTIONS_HPP. I'd recommend getting rid of the prepend_time functor with its $xx format and use the strftime-based one instead. It might be also be nice to provide another functor that uses Boost.DateTime for higher-resolution timestamps. append_enter should be renamed append_newline. Enter is not a chracter, it is a key on a keyboard. Likewise "enter" should be changed to "newline" throughout the docs. boost/log/extra directory. Does this name really make sense? Shouldn't this just be "appenders"? I won't comment on the quality of the code in here, since its mostly mine :-) There's actually a bug in my rolling_file_appender. The "for" loop in rolling_file_appender::rollover should read: for (int suffix = num_files - 2; suffix >= 0; --suffix) { Otherwise you end up with num_files + 1 files, not num_files. Also, in general, do the loggers really need the m_destroyed member? Why isn't the shared_ptr in m_impl enough to ensure the object lifetimes? I will need to do some tests to back this up, but I have an abiding concern that the overhead of all the string copying and resizing that results from the prepend operations will make logging more computationally expensive than it need be in the (IMHO) common case of prepending a timestamp and possibly thread ID to each message. It might make more sense for the modifiers to operate directly on output streams. Modifiers would receive a std::ostream& in addition to two strings and all output would be done on the stream. Just a thought, and I need to back this up with concrete profile results before I can claim anything other than a gut feeling. Jamfiles contain references to d:/boost/boost_latest with a comment "FIXME remove this!". Remove this :) There are no tests, though the examples exercise most of the functionality. Provide a Jamfile to build the examples. - What is your evaluation of the documentation? The documentation covers the important features of the library. Appearance-wise, I think the HTML pages look anachronistic. I'd suggest rewriting the documenatation using one of the meta-formats like QuickBook, ReStructured Text or the like to allow for a more Boost-like look and feel to the pages, as well as the possibility of rendering in formats other than HTML. In general I see a lot of (parenthetical phrases) that would be better set off by commas, not parentheses. Specific comments: motivation.html: "it's up to you how much information you log, and where.." Remove extra period. "The *Boost Log Library *is a small library, who:". Should be "which" or "that" (English majors?), but not "who". "allows for easy and *efficient *setting the level of logs/ log hierarchies<file:///U:/src/logging/boost_root/libs/log/doc/manipulating_logs.html#level>". Wording is awkward and at this point, the level concept has not been introduced. How about (new point) "allows messages to be logged with an associated level (e.g. debug, info, warning, error)", and then "allows logs to be easily configured to include or exclude messages of a given level." Personally I prefer the more verbose term "severity level" for this concept, but level is OK. logs.html: The first section should probably be named "Log Concepts". Very awkward paragraph: "*the appender(s) of the log*: where will messages that are written to this log be outputted to. For instance, you can have a log that outputs messages to the console and to a certain file at the same time." Instead, how about: "* Zero (one?) or more appenders. Appenders are functors that receive the messages you log and write them someplace. The Boost.Log library provides appenders for writing to a file, standard output, a debug window (Windows-specific), among others.". The word appenders should link to modifier_logger.html. Again: "*the modifier(s) of the message*: functions that modify the original message. Such functions can prefix the original message by the current time, and/or by the current thread's ID, append an enter to the message, etc." How about: "* Zero or more modifiers. Modifiers are functors that can be used to (suprise!) modify the messages that a user logs. Standard modifiers are provided for prepending a timestamp, thread ID, or append a newline to each message." The word modifiers should link to modifier_logger.html. And: "the log's *level*. When you write a message to the log, you specify its level. If the message's level is higher or equal to the the log's level, the message get written (to this log's destinations). Otherwise, the message is ignored (in fact it won't even be processed)" To: "The log's level. Users of the Boost.Log library specify a level with each logging call, or a default value is used. Only messages with a greater than or equal to the log's configured level will actually be logged [*] As a note or perhaps part of the paragraph above: "[*] Messages that are ignored are not even formatted, saving processing time. Because this check involves a simple numeric comparison, it is possible (and even recommended?) to leave debugging log statements in production code. They should not measurably affect your application's performance when they are disabled." Perhaps this would be a good place to discuss or link to a discussion of the compile-time enabled/disabled loggers. Declaring/Defining Logs: Change: In order to be used, logs need to first be declared (and defined somewhere). You'll declare logs in a header file: To: Logs must be both declared and defined before they may be used. In general, users will declare logs in a header file: modifier_logger.html: "append an enter to the message, if not already found". Should be "newline", not "enter". "prepend the log' name (example, if the log is named "app.charts.dbg", prepend this to the message)". Change to "prepend the log's name, for example "app.charts"". I think the use of "dbg" here is a remnant of the pre-levels design. manipulating logs.html: There are a number of cases in this section where the terminology gets muddled. Modifiers are called both modifiers and manipulators. Stick to a single name. "Note that if modifier functions A and B have the *same *index, they way they get called is unspecified (you don't know for sure if A is called before B, or vice-versa)". Change "way they get called" to "order in which they are called" and drop the parenthesized phrase. "The *name *can associate a modifier func with a certain name. This is only useful if you might want to later delete this modifier func". Should read "The methods that take a name argument can be used to associate a name with a modifier. This name can be used later to remove the modifier". Perhaps have "remove the modifier" as a link to the documentation for del_modifier. I like Daryl's proposed change to the way appenders are associated with logs. This might make sense for modifiers as well, and could make it quite easy to write a totally configuration-driven method to initialize the Boost.Log subsystem. However, doesn't this push thread-safety concerns into the appenders? For example, if several logs share a given appender, is it now up to the appender to serialize access to operator() instead of the log. - What is your evaluation of the potential usefulness of the library? Extremely useful. I plan to replace usage of the log4cplus library (a log4j port) and another home-grown logging library with Boost.Log in production code. - Did you try to use the library? With what compiler? Did you have any
problems?
Yes, with GNU g++ 3.3.4 on Linux. Some minor issues getting the Jamfile to work (had to add path to Boost.Log to <includes> and some compile warnings, but the code compiled and works. Here are the warnings from gcc: "g++" -c -Wall -ftemplate-depth-255 -g -O0 -fno-inline -I"../../../bin /boost_root/libs/log/build" -I"/home/nbde52d/src/boost" -I"../../.." -I "/home /nbde52d/src/boost" -o "../../../bin/boost_root/libs/log/build/libboost_log.a/g cc/debug/runtime-link-static/functions.o" "../src/functions.cpp" "/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug "../../../b in/boost_root/libs/log/build/libboost_log.a/gcc/debug/runtime-link-static/functi ons.o" ../src/functions.cpp: In constructor ` boost::logging::prepend_time::prepend_time(const boost::logging::logging_types::string&)': ../src/functions.cpp:133: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:135: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:138: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:138: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:139: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:144: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:146: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:148: warning: comparison between signed and unsigned integer expressions (all of the _idx variables should be of type logging_types::string::size_type; this is even typedef'd above) "g++" -c -Wall -ftemplate-depth-255 -g -O0 -fno-inline -I"../../../bin /boost_root/libs/log/build" -I"/home/nbde52d/src/boost" -I"../../.." -I "/home /nbde52d/src/boost" -o "../../../bin/boost_root/libs/log/build/libboost_log.a/g cc/debug/runtime-link-static/log.o" "../src/log.cpp" "/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug "../../../b in/boost_root/libs/log/build/libboost_log.a/gcc/debug/runtime-link-static/log.o" ../../../boost/log/log_impl.hpp: In constructor ` boost::logging::logger_impl::logger_impl(const boost::logging::logging_types::log_name_string_type&)': ../../../boost/log/log_impl.hpp:127: warning: ` boost::logging::logger_impl::m_is_compile_time' will be initialized after ../../../boost/log/log_impl.hpp:124: warning: `level_type boost::logging::logger_impl::m_level' ../src/log.cpp:73: warning: when initialized here gcc-C++-action ../../../bin/boost_root/libs/log/build/libboost_log.a/gcc/debug/r untime-link-static/log_manager.o (chage order of member initializers in ctor) The "multiple_threads" example would not compile properly without changing this line in use_logs.cpp: thread( bind(writer_thread,idx) ); to thread noname ( bind(writer_thread,idx) ); - How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
I've spent a good deal of time looking at this library in both its present form and at earlier versions. - Are you knowledgeable about the problem domain? Yes. I have used a number of logging APIs in a number of different languages, and implemented a couple myself. I vote that this library be accepted into Boost pending some work on the documentation and after addressing some very minor implementation issues. -- Caleb Epstein caleb dot epstein at gmail dot com