Re: [boost] [Review] The review of the Boost.Logging library starts today, November 7th

From: John Torjo (john.lists_at_[hidden])
Date: 2005-11-08 05:31:07

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!