Re: [boost] [Review] The review of the Boost.Logging library startstoday, November 7th (long)

From: John Torjo (john.lists_at_[hidden])
Date: 2005-11-11 09:05:10

Hi Pavel,

> 
> -------------------
> 
> the check for DLL:
> 
> #if defined(_DLL) || defined(_USRDLL)
> #ifndef BOOST_LOG_DYN_LINK
> #define BOOST_LOG_DYN_LINK
> #endif
> #endif
> 
> 
> would be better omitted.
> Logger may be used internally within DLL only
> and not exported. If user wants Boost-dynlink
> he should say it explicitly.
> 
> 

Myself, I love defaults, when they are provided. Instead of trying to 
find out how to enable something, I'd rather have it enabled by default, 
and eventually if I want to disable it, I will.

So, I could provide a way to disable this, with a define like 
BOOST_LOG_NO_DYN_LINK

> If the code is kept anyway then it should handle
> other compilers too,
> e.g. for BCB it is __DLL__.
> 
Yes, you're right.

> _____________________________________________________
> 3. log_manager.hpp: since this file is not intended
>    for include by user it should be moved into detail/
>    directory.
> 
Yup, done that.

> 
> -------------------
> 
> Name DEFAULT_CACHE_LIMIT should be
> DefaultCacheLimit or so to avoid preprocessor clashes.
> 

Will do.

> _____________________________________________________
> 4. detail/ts_win32.hpp: the functionality here is
>    possibly covered by Boost.Thread (in header only
>    form). If really so, it should be reused.
> 

:)
I allowed for this since others asked to remove dependency on Boost.Thread

> --------------
> 
> The #include <windows.h> may be replaced by few simple
> forward defines and declaration to speed up the
> compilation.

Yes.
> 
> _____________________________________________________
> 5. detail/ts_posix.hpp: the same advice related
>    to Boost.Thread as above.
> 

See above.

> I am suspicious of MT safety on multiprocessor machines
> due to use of unprotected "m_count" value.
> This value may be cached and changes to it not visible.
> 
> If possible, Boost.Thread should be reused to avoid
> such doubts.
> 

This was taken from Boost.Thread.

> _____________________________________________________
> 6. basic_usage.html: the code snippets would look better
>    if syntax highlighted.

Got it.

> -------------
> 
> Code snippets for header and implementation file should
> be more visually separated.

Will do.

> 
> --------------
> 
> Code snippets may have more comments. Overcommenting
> is positive thing, especially for first few examples.

Right, will do so.

> 
> _____________________________________________________
> 7. logs.html: there should be hyperlinks so one can
>    check out what appenders/modifiers interfaces are.
> 

Ok, will rewrite the docs in doxygen

> ---------------------
> 
> The code snippet with
> 
> BOOST_DEFINE_LOG(gui, "app.gui")
> 
> should say what the 'gui' is - object,
> "string name" or "C++ name"?

Done.

> 
> ---------------------
> 
> Possible feature of logger: it would be nice
> to have ability to automatically append newline
> (if not already at the end of a log).
> 
> Loggers I wrote do this and it helps since I
> do not need to search and fix source if I forget
> newline somewhere.

Already there : append_enter, which will be renamed : append_newline

> ---------------------
> 
> There should be table of all available modifiers
> and all available appenders at this moment.
> This table should contain one-line mini-examples.
> 

Yup, added to my TODO list.

> Modifier API:
> void modifier_func(const std::string & log_name, std::string &msg);
> 
> Is it really needed to use std::string for
> log_name? Woudn't it be enough to have const char*?
> 
> I ask because it may help to avoid one dynamic allocation
> per log and this may be quite important in RT applications.

The logger's name is kept in the logger_impl - so it's only one copied 
only once per log.

> 
> 
> -----------------------
> 
> Order in which appenders and modifiers are called is
> not specified.

In modifier_logger.html, at the end, I did explain the order. Maybe I 
can place a link to it, from a FAQ or so.

> 
> _____________________________________________________
> 9. manipulating_logs.html:
> 
> 
> "When manipulating logs, you alway manipulate a hierarchy of logs."
> (typo "alway")
> ==>>
> "You can manipulate with one or more logs at the same time."
> 
Yes, corrected.

> -------------------
> 
> DEFAULT_INDEX /cannot/ be uppercase (clashes with
> preprocessor).

I understand that it would better be lowercased, but "cannot" is a 
pretty strong word. Care to exemplify?

> 
> -------------------
> 
> What if I do:
> 
> manipulate_logs("*").xyz(....).abc(...);
> 
> manipulate_logs("*").abc(....).xyz(...);
> 
> 
> Will the last call overwrite the previous or will
> they accumulate? Should be explained here.

Will explain.

> 
> ----------------------
> 
> The rules where to use ampersand for appenders/modifiers
> should be the same. It is not acceptable sometimes to
> have it and sometimes not. This is really, really confusing.

They are the same.

> 
> Perhaps using "index" to sort modifiers is overengineering.
> 
> Intuitively I would expect sorting being derived from order
> of add_appender()/add_modifier() within manipulate_logs().
> 
> If you remove the index the API would get pleasantly smaller
> w/o loosing feature.

And the way it's now you don't have to care about the index unless you 
want to.

> ----------------------
> 
> The last code snippet doesn't say whether
> BOOST_LOG_DEFINE_LEVEL can be in header.
> 
> It also seems to put the value into
> boost::logging::level namespace which is wrong.

Why would you say this is wrong? I did this to prevent name clashes.

> 
> If the macro is already within a namespace
> it would insert new subnamespace boost/.... here
> making big mess in code.
> 
> 
> 
> Namespacing should be left to the user:
> 
> BOOST_LOG_DEFINE_LEVEL(my_namespace::xyz, 1111)
> 
> or
> 
> namespace my_namespace {
>   BOOST_LOG_DEFINE_LEVEL(xyz, 1111)
> }
> 

Yes, you're right - I need to explain this.

> _____________________________________________________
> 10. predefined.html:
> 
> append_enter ==>> append_newline
> 

Yes, will do - Caleb suggested as well.

> ----------------
> 
> prepend_thread_id should exists also for POSIX
> (there's one simple function for it).
> 

Please show it to me - I don't know it.

> --------------------
> Missing escape sequences:
> 
> $d - day with 1 or 2 digits
> $M - month with 1 or 2 digits
> $MON - month as Jan/Feb/....
> $ms - millisecond (it is quite useful for many common cases)
> $HH - like "03 AM"
> 

Too many things on my TODO list :)
I'll think about it.

> --------------------
> 
> ts_appender ==>> longer, more intuitive name
> 

Yes, you're right.
Perhaps: 'dedicated_thread_appender'.

> 
> "appender_array" - confusing documentation, no idea what is says.
> 

Yup, you're right. Need to fix that.

> 
> -----------------------
> 
> rolling_file_appender - example needed, picture would help.
> 
> 
> Name should be "rotating....".
> 

Again, you're right.

> 
> Dedicated thread used for logging: does it work
> if both application executable and a DLLs do log into
> the same file? It is quite tricky to ensure singleton
> in such circumstances. Jason Hise works on library "Singleton"
> where he had solved this (after a lot of effort).

I know it's been successfully used on Windows. Would 
Linux/some_other_platform be a problem?
> 
> ----------------------
> 
> Static destruction: currently the library cannot
> reliably log from destructors of static objects.
> The Jason's library contains support for this situation.
> 

Will look into it.

> 
> --------------------------
> 
> 
> It may be possible to design the BOOST_DECLARE_LOG
> and BOOST_DEFINE_LOG so that the log get initialized
> on first use: then the cache would not be needed.
> 
> This way should be investigated since possible
> simplification of design and API would be rather high.
> 

That's not the problem. The thing is that you need to specify the logs 
modifiers and appenders. So, no matter what, you might still have a few 
messages written to the logs, until you get a chance to initialize them.

You might be able to initialize them only within main() - for instance, 
from a command line parameter or so.

> 
> _____________________________________________________
> 11. Feature request: quite often I am not interested
>     not in absolute time (2005/12/11 16:33) but in
>     time offset from some event.
> 
> I suggest to add API:
> 
> manipulate_logs(...).set_time_start_point(
>    bool either_in_all_threads_or_just_in_current_thread
>    );
> 

Yes, it's a very reasonable thing to ask.

> _____________________________________________________
> 12. Feature request: thread ID (both Win32 and POSIX)
>     is of low use. I suggest to add API to specify
>     string name as alternative to number:
> 
> .prepend_thread_name()
> 
> manipulate_logs(...).set_current_thread_name("gui thread");
> 
> 
> If not specified number ID would be default.

Yup, doable.

> _____________________________________________________
> 13. Feature request: Boost.Filesystem should be supported
>     (for specifying file names). The library is stable
>     and likely in next Standard.
> 

There's no stoppping you from saying:
some_path.string();

> 
> 
> This is what was likely discussed all over: for
> some apps (e.g. embedded with tight constraints)
> I may wish to remove all logging code completely:
> 
> 
> BOOST_LOG(app, (<< "x = " << x));
> 
> Some compilers may not optimize strings and logging support
> aways from the code - they won't be called but they will
> be present in the executable. A way to surely get rid of them
> should exists.
> 

Right, will provide this.

> 
> _____________________________________________________
> 15. caching.html: the docs doesn't say whether flushed
>     logs follow the rules given by manipulate_logs().
> 

Done it.

> --------------
> 
> It doesn't say whether manual flush() is needed and what
> is default.
> 
> Complete example is sorely needed.
> 

Will do.

> 
> _____________________________________________________
> 16. thread_safe.html: this page is messy and I am not
>     able to understand what it is for and what it
>     tries to explain.
> 
> What is very missing is getting ensured that ALL logs no matter
> what are MT safe.

Funny, but that's what it says in the beginning "The Boost Log library 
is thread-safe, while being efficient."

> _____________________________________________________
> 19. examples.html: every example could be linked here,
>     together with short description.
> 
Will do.

> 
> _____________________________________________________
> 20. Feature: since each log may span to several lines
>     a modifier could be added to the library:
> 
>    .add_modifier(log_ending("###"))
> 
> 
> where every log will be ended with [optional \n if needed]###\n.
> 
> 
> This would allow easier create tools that manipulate
> with logs.
> 
> 
> Similar modifier:
>    .add_modifier(log_starting("@@@"))
> 
> where every log starts with @@@ (no newlines)
> may be considered but this one is not that urgent.
> 

I did not understand what you're saying.

> The documentation lacks reference for some important
> objects like manipulate_logs(). At least list of all
> standalone functions should exists.

Yes, there will be reference.

> The documentation should contain complete example how to write
> a) custom appender and modifier

I do have an example of creating a simple modifier & adppender 
(modifier_logger.html)

> Manipulating with one log can use single preallocated
> buffer and fall back to heap if needed. Even if it would
> complicate API for appenders/modifiers it feels as worth.
> 
> 
> It may be worth to create your own "preallocated_string"
> class and work with it instead of with std::string.
> 

I'll think about it.

> 26. Similary to OutputDebugString() on Win32 with POSIX
>     saving to syslog may be implemented.
> 

I need help with this one...

>     On Win32 EventLog suppot may be added (very handy
>     for errors).
> 

... same here

> _____________________________________________________
> 27. There should be way to disable thread locking
>     manually when useful (e.g. via macro). For some
>     people with single thread app such overhead may
>     feel too high.

There is. If you disable threads (BOOST_HAS_THEADS is not defined), 
there's no thread locking.

> 
> _____________________________________________________
> 28. src/log_manager.cpp: name_matches_spec() may be more optimised:
> 
>   if ( (spec == "*") || spec.empty() )
> 
> ==>>
> 
>   if (spec.empty() || strcmp(spec.c_str(), "*") == 0)
> 
> -> one dynamic allocation less.

spec == "*" -> is usually optimized by providing the right operator==, 
and that's the STL vendor's business

For instance:
template<class _Elem,
        class _Traits,
        class _Alloc> inline
        bool __cdecl operator==(
                const basic_string<_Elem, _Traits, _Alloc>& _Left,
                const _Elem *_Right)
        {	// test for string vs. NTCS equality
        return (_Left.compare(_Right) == 0);
        }

> ----------------
> 
> Length of lines may be limited.
> Some lines have over 240 characters and that
> very hard to read.
> I'd say that 90-100 chars is reasonable limit.
> 
Yes, I need to improve on that.

> -----------------------
> 
> The file log_impl.hpp should be wrapped by:
> 
> #include <boost/detail/workaround.hpp>
> 
> 
> #if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564))
> #  pragma warn -8026 // some functions are not expanded inline
> #endif
> 
> .... file contents ....
> 
> #if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564))
> #  pragma warn .8026
> #endif
> 
> 
Done. thanks.

> _____________________________________________________
> 31. log.hpp: the code to find out release mode
> 
> #ifndef NDEBUG
> #define BOOST_LOG_IS_DEBUG_MODE 1
> #else
> #define BOOST_LOG_IS_DEBUG_MODE 0
> #endif
> 
> 
> doesn't work correctly on BCB. Here IDE uses
> by default _DEBUG macro. I suggest:
> 
> 
> #if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564))
> // IDE of BCB uses _DEBUG to indicate debug mode, it doesn't
> // (by default) use NDEBUG.
> #  ifdef _DEBUG
> #    define BOOST_LOG_IS_DEBUG_MODE 1
> #  else
> #    define BOOST_LOG_IS_DEBUG_MODE 0
> #  endif
> #else
> #  ifdef NDEBUG
> #    define BOOST_LOG_IS_DEBUG_MODE 0
> #  else
> #    define BOOST_LOG_IS_DEBUG_MODE 1
> #  endif
> #endif

Done, thanks.

Thanks for the thorough review.

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!