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!