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