[boost] Logging library review

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2008-02-17 02:50:01

AMDG

General:

I notice that you are using the <stdlib.h> includes
rather than <cstdlib> e.g..  Is there a good reason?

I've noticed a couple of places that have a comment before the
include guard saying "this is fixed".  That's hardly an
informative message unless I know what was wrong with it.

Individual Files:

defaults.hpp

If you want to override any of the above, you should do the following:
- before including anything from Boost Logging Library, <tt>\#include 
<boost/logging/defaults.hpp> </tt>
- override the types
- do <tt>\#include <boost/logging/logging.hpp> </tt>

IMO, this is a very dangerous way to go.  I would prefer something like:
- create a header that defines the correct typedefs.
- #define BOOST_LOGGING_CONFIG_HEADER path/to/override/header

logging.hpp:
    No comments.

format.hpp:
use_cache::format:
  Can you change this to do the check and insertion in a single step?
  if(m_formats.insert( fmt).second)

"@param lock_resource_type What class you use to do allow thread-safe 
access to an instance of this clas (used internally)."
Missing an "s" in class

format.hpp needs to #include <algorithm>
del_fomatter/destination.  Could you spell out delete? It's only three
more letters.  Alternately it could be erase_formatter

msg_route::simple vs. msg_route::with_route.
Why does simple ignore it's constructor arguments
while with_route stores references to them?

with_route::route_do_set/route_do_append.  This is *evil*.
Don't put anything that can throw in any destructor.

profile.hpp:

scoped_compute::~scoped_compute() If 
::boost::posix_time::microsec_clock::local_time()
throws this is bad.
Also, for multithreaded programs, you don't want the elapsed time,
you want the time spent executing the current thread.
Is there a way to do this?  Otherwise this
weakness should be documented.

detail/after_being_destroyed.hpp
Why don't you delete this file?

detail/cache_before_init.hpp
You're using the thread id stuff in another place.  Could you
factor it out into the threading directory?

You already have thread specific storage. Use it!

detail/cache_before_init_macros.hpp

I really like the idea of caching.  I've had to deal with
the problem of trying to figure out where to log to, when
I might have to log before main and this solves the problem
neatly.

detail/error.hpp
Why?

detail/filter.hpp
  no comments.

detail/find_format_write.hpp
  no comments.

detail/find_gather.hpp
  no comments.

detail/format_fwd_detail.hpp
  no comments.

detail/format_msg_type.hpp
  no comments.

detail/format_write_detail.hpp
  no comments.

detail/forward_constructor.hpp

BOOST_LOGGING_FORWARD_CONSTRUCTOR_WITH_NEW for msvc:
Please rearrange this to use the constructor rather than
using assignment.

detail/fwd.hpp

    /*
        just in case you're doing a typo - "write" instead of "writer"
    */
    namespace write = writer;
I beg you're pardon!?

using namespace destination;
file f("out.txt", file_settings.initial_overwrite(true).do_append(false) );

Is file_settings actually a global variable?  Oh.  Should this be
file_settings()?  That looks better.

flag_with_self_type::operator=(const self_type & other)
I don't understand what this overload accomplishes.  m_val
is certainly not a member of file_settings for example.

detail/level.hpp
  Am I allowed to make my own levels?
  static const int my_level = 221;
  If I did it would need to go in namespace boost::logging::level
  right?  Is there anything else special about levels?

holder_ts
There is no reason to make the typedefs scoped_lock and mutex public.
The mutex object is private anyway.

detail/log_keeper.hpp:
logger_holder:
  Since you're defining operator-> why don't you define operator* too.

ensure_early_log_creation
I can see the point of this class, but why all the
mess in the implementation?  I don't see at all how
it helps.

detail/logger.hpp

gather_holder:
I don't like the use of mutable.  Is it necessary?
Would the auto_ptr trick do?

logger<gather_msg, default_ >::~logger():
turn_cache_off???  Can this throw?  Should the messages be written
to some unspecified location or discarded?  Should the behavior
be more configurable in this pathological case?

forward_to_logger:
        // ... might be called for a specialization of logger - for 
logger<gather_msg,write_msg*>
        typedef typename boost::remove_pointer<write_msg>::type write_type;
This isn't going to be surprising at all?

detail/logger_base.hpp

I notice that logger_base inherits from types in the detail
namespace.  This means the ADL will look in namespace detail
for loggers.  Is that a problem?

detail/macros.hpp

Why does BOOST_LOG_COMPILE_FAST cause the logger_holder to be
returned by reference, while the otherwise the logger is
returned by reference?

manipulator.hpp

Use #pragma warning push/pop

line 513
Thus, usually generic manipulators have a templated operator=, and do 
the best to convert what's in, to what they need.
Do you mean templated operator()?

Going back to my criticism of manipulators, here is basically the
interface I would like:

concept Manipulator<class M, class StringType> {
    void operator()(const M&, const StringType&);
    bool operator==(const M&, const M&) { 
return(boost::is_empty<M>::value); }
    void configure(const hold_string_type&) {}
}

Detecting the presence or absence of operator== can be a pain
and may not work in all cases.  Detecting configure can be done
easily and I would definitely go for it to make the interface
simpler.  Oh.  named_spacer depends on the presence of the
typedef convert_type, too.  *Sigh*  Does this mean that the
example is wrong:

template<class convert_dest = do_convert_destination > struct cout {
    template<class msg_type> void operator()(const msg_type & msg) const {
        convert_dest::write(msg, std::cout);
    }
};

Also, if I understand correctly only the named_* stuff
depends on configure.

Since you have to create a pointer anyway for polymorphism
it shouldn't cost any more to always treat manipulators
as generic.

detail/scenario.hpp
  no comments

detail/scoped_log.hpp

BOOST_SCOPED_LOG_CTX_IMPL
Make sure the destructor can't ever throw.

detail/tags.hpp
  no comments.

detail/time_format_holder.hpp:
the two versions of write_time pass different numbers of
arguments to sprintf.  There is no check to make sure that
the format string is correct.  If I use nanosecs
with plain time the error will manifest itself in undefined
behavior.  Why don't you pad with 0's
line 162:
        sprintf( buffer, m_format.c_str(), vals[1], vals[2], vals[3], 
vals[4], vals[5], vals[6], vals[7], 0, 0, 0 );

detail/logger_format_write.hpp
  no comments.

detail/util.hpp

would compute msg_type right now; however, we want the compiler to wait, 
until the user has actually set the msg_type,
for example, using the BOOST_LOG_FORMAT_MSG macro. Thus, we do:

This really bothers me.  I don't think that you should rely on
this kind of magic for non-dependent types.  If you want to do
it use template paramters for configuration.  Otherwise use,
preprocessor symbols to guarantee that the requisite specializations
are available soon enough.

format/array.hpp
shared_ptr_holder::append:
You should construct a shared_ptr outside the mutex so that
you don't need to allocate any memory inside the critical
section.

format/named_write.hpp
  No comments
format/named_write_fwd.hpp
  No comments

format/op_equal.hpp
I don't like the restriction that operator== needs to be a member
function.  I think I can see why you are doing it (To prevent
infinite recursion, right?) but still, there ought to be a better
way.

optimize.hpp:
I notice that cache_string_one_str grows linearly.
I thought that the general way was to grow exponentially?

The code is litered with explicit casts to int.  Wouldn't
it be better to use std::size_t and make sure it's safe?

I'm not entirely convinced that cache_string_one_str will
be faster than std::string at all.  It can certainly be
made quite a bit faster than it is.

cache_one_str::to_stream seems to require that the stream
be opened in binary mode?

I'm confused about the relationship between cache_string_one_str
and cache_string_several_str.  They don't have the same interface.
In addition, prepend_string behaves differently between the two.
for cache_string_one_str prepend_string causes it's argument
two be prepended.  For cache_string_several_str it causes
it's argument to be inserted at the end of the set of prepended
strings.  Is this difference intentional?  Further, 
cache_string_one_str::set_string
resets the entire string.  cache_string_several_str::set_string only
resets the message string--nothing that has been prepened or
appended.

format/destination/convert_destination.hpp

300 column lines is a bit excessive, don't you think?

do_convert_destination? Is the name convert_destination
already taken?   I normally think of names
like this as being implementation names rather than
interface names.

format/destination/defaults.hpp
I'd like to see this broken up into separate files.
#include <boost/logging/format/destination/iostream.hpp>
#include <boost/logging/format/destination/ostream.hpp>
#include <boost/logging/format/destination/dbg_window.hpp>

why is file.hpp included?

format/destination/file.hpp

#pragma warning ( disable : 4355)
Whatever happened to #pragma warning push/pop?
Shouldn't you check for BOOST_MSVC instread
of _MSC_VER.  This should be /after/ the
#includes.

format/destination/named.hpp
same comment as for file.hpp about #pragma warning

destination::detail::named_context::add:
What happens if I attempt to use the same name
more than once?  Is it simply not allowed?
It looks like what actually happens is that
the old destination will still be present but the
name will now refer to the new destination.  Is that
your intent?

comment before named_context::compute_write_steps:
        // recomputes the write steps - note taht this takes place after 
each operation
s/taht/that/

named_t::named_t.  Should this be explicit?

named_t::string.  I would like this better if it were
called set_format.

I would like named().add("cout", cout()) to have cout on
by default.

format/destination/rolling_file.hpp
#pragma warning again.

rolling_file_info::rolling_file_info.
                // so that we don't get exceptions
                fs::path::default_name_check( fs::no_check);
I don't think you should change global filesystem settings.
Especially since this is deprecated!

rolling_file_info::recreate_file:
Is it OK to open a file that you already have open, again?

I'd like to be able to control the names of the
individual files so that I can have
file1.log file2.log &c. e.g.

format/destination/shared_memory.hpp
  Since this is not working I'll leave it alone.

format/destination/syslog.hpp
  No comments.

format/formatter/convert_format.hpp
string_finder< 
::boost::logging::tag::holder<string,p1,p2,p3,p4,p5,p6,p7,p8,p9,p10> >::get
Shouldn't this invoke string_finder< string>::get rather than relying on
an implicit conversion?

In namespace modify:
        template<class string> void write(string_type & src, 
boost::logging::optimize::cache_string_one_str<string> & dest) {
            dest.set_string_swap(src);
        }
First of all set_string_swap is a member of cache_string_several_str not of
cache_string_one_str. Second, I think this is dangerous.  Users will expect
that write(src, dest) will behave the same if src is const vs. non-const
provided that it works in both cases.  Wait to rvalue references to make
this optimization.

do_convert_format:
Is there a reason for the separate overloads of write
for const char_type *?

format/formatter/defaults.hpp
You might break this up into separate headers.

format/formatter/high_precision_time.hpp
Is 64 guaranteed to be enough characters?
Oh.  I see the assertion in time_format_holder.hpp.
Ever thought about using named constants?

high_precision_time_t::write_high_precision_time::write_high_precision_time
In the switch you might add an assertion
        case 0: assert(nanosecs == 0); break; // no high precision at all

format/formatter/named_spacer.hpp
named_spacer_context::write_with_convert(msg_type & msg, ...):
Should this really use 
::boost::logging::formatter::do_convert_format::append*?

The use of context bothers me.  Suppose that I stick a formatter
that appends into a named_spacer that prepends?  What happens?
Is there any way to detect such a condition?

I think the problem is that you are trying to mix two different
abstractions, here.  The formatters represent a generic
modification to the string and that's fine.  The named_spacer
allows printf like formatting at runtime with function objects
to fill in the placeholders.  That's fine too.  The problem is
that formatters do not simply return some text which the named_spacer
can insert in the correct position.

also I'd like to be able to specify format strings like: "[%idx%] %time% 
%msg%\n"
to control the message placement as well.

named_spacer_context::compute_write_steps
You might be helpful and give an error if there are unpaired %'s

format/formatter/spacer.hpp
I'm glad to see that convert_type is propogated correctly.

format/formatter/tags.hpp
uses_tag could use more specific documentation.  In particular
it needs to say that the derived class should have a member
called write_tag and that uses_tag supplies the operator()

format/formatter/thread_id.hpp
  no comments

format/formatter/time.hpp
  no comments

format/formatter/time_strf.hpp
Is there any particular reason that this doesn't support runtime 
configuration?

gather/ostream_like.hpp
Doesn't need to #include <iostream>

out_base won't be needed anymore, right?

out_of_the_box/use_levels.hpp and
out_of_the_box/use_levels.hpp
I notice that these don't use ensure_early_log_creation.

tag/defaults.hpp
thread_id doesn't handle BOOST_HAS_MPTASKS although 
format/formatter/thread_id.hpp
does.  Is there a reason for the discrepency?
needs to #include <ctime>
Where is ::boost::logging::level::type defined.  Ah detail/level.hpp
This is the file that you need to #include.

tag/high_precision_time.hpp
Should this support configure()?

writer/format_write.hpp
  No comments.

writer/named_write.hpp

lines 296-298
  In this case you already know that
(a) the string is not empty
(b) the last character is a '%'
(c) the first character is a '%'  (invariant given a.)

The only thing that could cause the test has_manipulator_name()
to be false is if m_manipulator is "%", in this case the
assignment does nothing.  This test can be replaced with
assert(has_manipulator_name() || m_manipulator == "%");

there are functions replace_formatter and replace_destination
I don't see a way to add formatters or destinations.

replace_formatter seems dangerous.  The correctness depends on the
(compile time known) convert_type of the new formatter matching
the (runtime known) format string.  Also, this (contrived) example won't 
work:

writer.format("[%idx%] |\n");
writer.replace_formatter("time", high_precision_time("..."));
writer.format("[%idx%] %time% |\n");

Not to mention that if I then change the format string again:
writer.format("[%idx%] | %time%\n");
it will revert to the old version of time.

In short, I think that you should either disallow the replacement
entirely or make it general.

on_dedicated_thread.hpp:
on_dedicated_thread::pause(): what ever happened to condition variables?

The inheritence from base_type worries me.  If base_type only provides
operator() then the inheritence is unnecessary.  If base_type provides
other functions then they probably shouldn't be called without locking.
It's dangerous to expose a partly thread safe interface.

writer/ts_write.hpp
ts_write_context declares the mutex mutable.  dedicated_thread_context does
not.  Which is right?
Again I am worried by the inhertance only more so.  on_dedicated_thread
might be used in an otherwise single threaded program.  ts_write probably
wouldn't be.

Whew.  That being said I vote to accept the logging library.
All the issues with destructors and exceptions absolutely must be
fixed.

In Christ,
Steven Watanabe