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

From: Caleb Epstein (caleb.epstein_at_[hidden])
Date: 2005-11-08 11:30:07

On 11/8/05, John Torjo <john.lists_at_[hidden]> wrote:

> > - 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.

In which case your object construction will fail in the member initializers
and not the body. Its the same effect - the caller will get a std::bad_alloc
and the object will not be created. I don't see how initializing the member
twice changes this.

> - 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;

I just meant that there are older definitions left in the code, commented
out, above the current definitions. Is this what you want?

from log.h (w/line numbers):

390://#define BOOST_SCOPEDLOGL(log_name,lvl) if
(::boost::logging::simple_logger_keeper keep =
::boost::logging::simple_logger_keeper(log_name,::boost::logging::level::lvl)
) ; else (*keep.stream())
391:#define BOOST_SCOPEDLOGL(log_name,lvl) if
(::boost::logging::simple_logger_keeper keep =
::boost::logging::simple_logger_keeper(::boost::logging::logger_and_level(log_name,::boost::logging::level::lvl))
) ; else (*keep.stream())

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.

But they're not macros. All-uppercase just screams macro to me.

> > 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.

I guess its just a point of preference, but I'm very familiar with strftime
and its format strings, and it is part of the Standard C Library. Not sure
which code you're referring to, but the implementation of prepend_time_stf
is only about 15 lines.

> 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)

This might be an argument of making the BOOST_LOG* macros work like this
then:

BOOST_LOG(logger, msg << to << stream)

so that the implementation could hold a shared pointer to the logger for the
lifetime of the call. Just a thought.

Thanks!

Welks :)

--
Caleb Epstein
caleb dot epstein at gmail dot com