Hi Caleb,
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.
Yes, on my TODO list.
boost/log/log.hpp:
- Should BOOST_LOG_DEFINE_LEVEL use BOOST_STATIC_CONSTANT?
Will be.
- The "default_" level should be renamed "default_level".
Added an extra alias called 'default_level'.
- enabled_logger::enabled_logger (logger&): initialize m_stream in the member initialization or the ctor body but not both
Initialized it in member initialization for just in case 'm_stream = new logging_types::stream ;' could throw.
- Remove old versions of macro definitions that have been rewritten (BOOST_IS_LOG_ENABLED, BOOST_SCOPED_LOGL).
BOOST_IS_LOG_ENABLED - I wanted to still allow this for when you want to clearly state you're talking about a log. I think it's no harm. BOOST_SCOPEDLOGL - when you want to use a scoped log, and instead of saying: if ( gui(my_level) ) gui.stream() << blabla; you can say: BOOST_SCOPEDLOGL(gui,my_level) << blabla; Again, I don't think it hurts, for consistency's sake, at least.
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.
The reason they're uppercased is because they're constants.
boost/log/functions.hpp: Macro guard looks like the victim of a Find/Replace gone awry: JT_BOOST_appender_funcTIONS_HPP.
Touche ;) Fixed.
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
The reason I prefer the prepend_time is because it's more straightforward IMO to say $dd (day), $MM (month), $yy (year 2 digits), $yyyy (year 4 digits) rather than %d (day), %m (month), %y (year 2 digits), %Y (year 4 digits). And the code is much easier to read, again, IMO.
another functor that uses Boost.DateTime for higher-resolution timestamps.
Yes, that would be nice ;) Added it to my TODO list
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.
Touche again ;) Will do.
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,
Yes, you're right. Will do.
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?
It's because internally, when writing BOOST_LOG(x) you're dealing with a function that returns a static reference. The problem comes when you're using the static reference after it's been destroyed (thus the shared_ptr would be invalid)
Jamfiles contain references to d:/boost/boost_latest with a comment "FIXME remove this!". Remove this :)
True... Done it.
There are no tests, though the examples exercise most of the functionality. Provide a Jamfile to build the examples.
Will do.
- 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.
Done.
"The *Boost Log Library *is a small library, who:". Should be "which" or "that" (English majors?), but not "who".
Done.
"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.
Done.
logs.html: The first section should probably be named "Log Concepts". Done
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.".
Yup, thanks. Done.
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.
Done.
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:
Done.
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.
Done.
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.
Done.
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.
I don't think that would be a problem. The way I see it, only the interface changes.
- 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.
Thanks! Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!