[Review] The review of the Boost.Logging library starts today, November 7th
Hi all, today starts the review of the Boost.Logging library written by John Torjo. It will end on November 16th. --------------------------------------------------- About the library: As applications are becoming more and more complex, logging is more and more of a must, in nowadays software. The Boost Logging Library is a library that: * makes it very easy to declare/define logs * allows you very simple access to logs * allows you to easily use/manipulate log levels (like, dbg/err/info/etc.) * allows for log hierarchies, and for manipulating log hierarchies * is thread-safe To get you started, here's some basic usage: BOOST_LOGL(app,dbg) << "debugging: " << i << '-' << j << std::endl; BOOST_LOG(app) << "I just wanted to tell you something...."; BOOST_LOGL(app,err) << "this is an error!"; The latest version of the lib can be found here: http://torjo.com/code/logging.zip --------------------------------------------------- Please always state in your review, whether you think the library should be accepted as a Boost library! Additionally please consider giving feedback on the following general topics: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? Regards Hartmut Review Manager
On 11/6/05, Hartmut Kaiser <hartmut.kaiser@gmail.com> wrote: - What is your evaluation of the design? I think the design is overall quite good. I like the named logger concept and the run-time configurability of the system is a great strength. - What is your evaluation of the implementation? I have some questions and suggestions: The namespace has been changed from the original poposed versions from "log" to "logging" but the subdirectory names remain "log". This should be reconciled. boost/log/log.hpp: - Should BOOST_LOG_DEFINE_LEVEL use BOOST_STATIC_CONSTANT? - The "default_" level should be renamed "default_level". - enabled_logger::enabled_logger (logger&): initialize m_stream in the member initialization or the ctor body but not both - Remove old versions of macro definitions that have been rewritten (BOOST_IS_LOG_ENABLED, BOOST_SCOPED_LOGL). 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. boost/log/functions.hpp: Macro guard looks like the victim of a Find/Replace gone awry: JT_BOOST_appender_funcTIONS_HPP. 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 another functor that uses Boost.DateTime for higher-resolution timestamps. append_enter should be renamed append_newline. Enter is not a chracter, it is a key on a keyboard. Likewise "enter" should be changed to "newline" throughout the docs. boost/log/extra directory. Does this name really make sense? Shouldn't this just be "appenders"? I won't comment on the quality of the code in here, since its mostly mine :-) There's actually a bug in my rolling_file_appender. The "for" loop in rolling_file_appender::rollover should read: for (int suffix = num_files - 2; suffix >= 0; --suffix) { Otherwise you end up with num_files + 1 files, not num_files. 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? I will need to do some tests to back this up, but I have an abiding concern that the overhead of all the string copying and resizing that results from the prepend operations will make logging more computationally expensive than it need be in the (IMHO) common case of prepending a timestamp and possibly thread ID to each message. It might make more sense for the modifiers to operate directly on output streams. Modifiers would receive a std::ostream& in addition to two strings and all output would be done on the stream. Just a thought, and I need to back this up with concrete profile results before I can claim anything other than a gut feeling. Jamfiles contain references to d:/boost/boost_latest with a comment "FIXME remove this!". Remove this :) There are no tests, though the examples exercise most of the functionality. Provide a Jamfile to build the examples. - What is your evaluation of the documentation? The documentation covers the important features of the library. Appearance-wise, I think the HTML pages look anachronistic. I'd suggest rewriting the documenatation using one of the meta-formats like QuickBook, ReStructured Text or the like to allow for a more Boost-like look and feel to the pages, as well as the possibility of rendering in formats other than HTML. In general I see a lot of (parenthetical phrases) that would be better set off by commas, not parentheses. Specific comments: motivation.html: "it's up to you how much information you log, and where.." Remove extra period. "The *Boost Log Library *is a small library, who:". Should be "which" or "that" (English majors?), but not "who". "allows for easy and *efficient *setting the level of logs/ log hierarchies<file:///U:/src/logging/boost_root/libs/log/doc/manipulating_logs.html#level>". Wording is awkward and at this point, the level concept has not been introduced. How about (new point) "allows messages to be logged with an associated level (e.g. debug, info, warning, error)", and then "allows logs to be easily configured to include or exclude messages of a given level." Personally I prefer the more verbose term "severity level" for this concept, but level is OK. logs.html: The first section should probably be named "Log Concepts". Very awkward paragraph: "*the appender(s) of the log*: where will messages that are written to this log be outputted to. For instance, you can have a log that outputs messages to the console and to a certain file at the same time." Instead, how about: "* Zero (one?) or more appenders. Appenders are functors that receive the messages you log and write them someplace. The Boost.Log library provides appenders for writing to a file, standard output, a debug window (Windows-specific), among others.". The word appenders should link to modifier_logger.html. Again: "*the modifier(s) of the message*: functions that modify the original message. Such functions can prefix the original message by the current time, and/or by the current thread's ID, append an enter to the message, etc." How about: "* Zero or more modifiers. Modifiers are functors that can be used to (suprise!) modify the messages that a user logs. Standard modifiers are provided for prepending a timestamp, thread ID, or append a newline to each message." The word modifiers should link to modifier_logger.html. And: "the log's *level*. When you write a message to the log, you specify its level. If the message's level is higher or equal to the the log's level, the message get written (to this log's destinations). Otherwise, the message is ignored (in fact it won't even be processed)" To: "The log's level. Users of the Boost.Log library specify a level with each logging call, or a default value is used. Only messages with a greater than or equal to the log's configured level will actually be logged [*] As a note or perhaps part of the paragraph above: "[*] Messages that are ignored are not even formatted, saving processing time. Because this check involves a simple numeric comparison, it is possible (and even recommended?) to leave debugging log statements in production code. They should not measurably affect your application's performance when they are disabled." Perhaps this would be a good place to discuss or link to a discussion of the compile-time enabled/disabled loggers. Declaring/Defining Logs: Change: In order to be used, logs need to first be declared (and defined somewhere). You'll declare logs in a header file: To: Logs must be both declared and defined before they may be used. In general, users will declare logs in a header file: modifier_logger.html: "append an enter to the message, if not already found". Should be "newline", not "enter". "prepend the log' name (example, if the log is named "app.charts.dbg", prepend this to the message)". Change to "prepend the log's name, for example "app.charts"". I think the use of "dbg" here is a remnant of the pre-levels design. manipulating logs.html: There are a number of cases in this section where the terminology gets muddled. Modifiers are called both modifiers and manipulators. Stick to a single name. "Note that if modifier functions A and B have the *same *index, they way they get called is unspecified (you don't know for sure if A is called before B, or vice-versa)". Change "way they get called" to "order in which they are called" and drop the parenthesized phrase. "The *name *can associate a modifier func with a certain name. This is only useful if you might want to later delete this modifier func". Should read "The methods that take a name argument can be used to associate a name with a modifier. This name can be used later to remove the modifier". Perhaps have "remove the modifier" as a link to the documentation for del_modifier. I like Daryl's proposed change to the way appenders are associated with logs. This might make sense for modifiers as well, and could make it quite easy to write a totally configuration-driven method to initialize the Boost.Log subsystem. However, doesn't this push thread-safety concerns into the appenders? For example, if several logs share a given appender, is it now up to the appender to serialize access to operator() instead of the log. - What is your evaluation of the potential usefulness of the library? Extremely useful. I plan to replace usage of the log4cplus library (a log4j port) and another home-grown logging library with Boost.Log in production code. - Did you try to use the library? With what compiler? Did you have any
problems?
Yes, with GNU g++ 3.3.4 on Linux. Some minor issues getting the Jamfile to work (had to add path to Boost.Log to <includes> and some compile warnings, but the code compiled and works. Here are the warnings from gcc: "g++" -c -Wall -ftemplate-depth-255 -g -O0 -fno-inline -I"../../../bin /boost_root/libs/log/build" -I"/home/nbde52d/src/boost" -I"../../.." -I "/home /nbde52d/src/boost" -o "../../../bin/boost_root/libs/log/build/libboost_log.a/g cc/debug/runtime-link-static/functions.o" "../src/functions.cpp" "/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug "../../../b in/boost_root/libs/log/build/libboost_log.a/gcc/debug/runtime-link-static/functi ons.o" ../src/functions.cpp: In constructor ` boost::logging::prepend_time::prepend_time(const boost::logging::logging_types::string&)': ../src/functions.cpp:133: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:135: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:138: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:138: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:139: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:144: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:146: warning: comparison between signed and unsigned integer expressions ../src/functions.cpp:148: warning: comparison between signed and unsigned integer expressions (all of the _idx variables should be of type logging_types::string::size_type; this is even typedef'd above) "g++" -c -Wall -ftemplate-depth-255 -g -O0 -fno-inline -I"../../../bin /boost_root/libs/log/build" -I"/home/nbde52d/src/boost" -I"../../.." -I "/home /nbde52d/src/boost" -o "../../../bin/boost_root/libs/log/build/libboost_log.a/g cc/debug/runtime-link-static/log.o" "../src/log.cpp" "/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug "../../../b in/boost_root/libs/log/build/libboost_log.a/gcc/debug/runtime-link-static/log.o" ../../../boost/log/log_impl.hpp: In constructor ` boost::logging::logger_impl::logger_impl(const boost::logging::logging_types::log_name_string_type&)': ../../../boost/log/log_impl.hpp:127: warning: ` boost::logging::logger_impl::m_is_compile_time' will be initialized after ../../../boost/log/log_impl.hpp:124: warning: `level_type boost::logging::logger_impl::m_level' ../src/log.cpp:73: warning: when initialized here gcc-C++-action ../../../bin/boost_root/libs/log/build/libboost_log.a/gcc/debug/r untime-link-static/log_manager.o (chage order of member initializers in ctor) The "multiple_threads" example would not compile properly without changing this line in use_logs.cpp: thread( bind(writer_thread,idx) ); to thread noname ( bind(writer_thread,idx) ); - How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
I've spent a good deal of time looking at this library in both its present form and at earlier versions. - Are you knowledgeable about the problem domain? Yes. I have used a number of logging APIs in a number of different languages, and implemented a couple myself. I vote that this library be accepted into Boost pending some work on the documentation and after addressing some very minor implementation issues. -- Caleb Epstein caleb dot epstein at gmail dot com
Hi Caleb,
I have some questions and suggestions:
The namespace has been changed from the original poposed versions from "log" to "logging" but the subdirectory names remain "log". This should be reconciled.
Yes, on my TODO list.
boost/log/log.hpp:
- Should BOOST_LOG_DEFINE_LEVEL use BOOST_STATIC_CONSTANT?
Will be.
- The "default_" level should be renamed "default_level".
Added an extra alias called 'default_level'.
- 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.
- 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; you can say: BOOST_SCOPEDLOGL(gui,my_level) << blabla; 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.
boost/log/functions.hpp: Macro guard looks like the victim of a Find/Replace gone awry: JT_BOOST_appender_funcTIONS_HPP.
Touche ;) Fixed.
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.
another functor that uses Boost.DateTime for higher-resolution timestamps.
Yes, that would be nice ;) Added it to my TODO list
append_enter should be renamed append_newline. Enter is not a chracter, it is a key on a keyboard. Likewise "enter" should be changed to "newline" throughout the docs.
Touche again ;) Will do.
boost/log/extra directory. Does this name really make sense? Shouldn't this just be "appenders"? I won't comment on the quality of the code in here,
Yes, you're right. Will do.
since its mostly mine :-) There's actually a bug in my rolling_file_appender. The "for" loop in rolling_file_appender::rollover should read:
for (int suffix = num_files - 2; suffix >= 0; --suffix) {
Otherwise you end up with num_files + 1 files, not num_files.
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)
Jamfiles contain references to d:/boost/boost_latest with a comment "FIXME remove this!". Remove this :)
True... Done it.
There are no tests, though the examples exercise most of the functionality. Provide a Jamfile to build the examples.
Will do.
- What is your evaluation of the documentation?
The documentation covers the important features of the library. Appearance-wise, I think the HTML pages look anachronistic. I'd suggest rewriting the documenatation using one of the meta-formats like QuickBook, ReStructured Text or the like to allow for a more Boost-like look and feel to the pages, as well as the possibility of rendering in formats other than HTML.
In general I see a lot of (parenthetical phrases) that would be better set off by commas, not parentheses. Specific comments:
motivation.html: "it's up to you how much information you log, and where.." Remove extra period.
Done.
"The *Boost Log Library *is a small library, who:". Should be "which" or "that" (English majors?), but not "who".
Done.
"allows for easy and *efficient *setting the level of logs/ log hierarchies<file:///U:/src/logging/boost_root/libs/log/doc/manipulating_logs.html#level>". Wording is awkward and at this point, the level concept has not been introduced. How about (new point) "allows messages to be logged with an associated level (e.g. debug, info, warning, error)", and then "allows logs to be easily configured to include or exclude messages of a given level." Personally I prefer the more verbose term "severity level" for this concept, but level is OK.
Done.
logs.html: The first section should probably be named "Log Concepts". Done
Very awkward paragraph:
"*the appender(s) of the log*: where will messages that are written to this log be outputted to. For instance, you can have a log that outputs messages to the console and to a certain file at the same time."
Instead, how about:
"* Zero (one?) or more appenders. Appenders are functors that receive the messages you log and write them someplace. The Boost.Log library provides appenders for writing to a file, standard output, a debug window (Windows-specific), among others.".
Yup, thanks. Done.
The word appenders should link to modifier_logger.html.
Again:
"*the modifier(s) of the message*: functions that modify the original message. Such functions can prefix the original message by the current time, and/or by the current thread's ID, append an enter to the message, etc."
How about:
"* Zero or more modifiers. Modifiers are functors that can be used to (suprise!) modify the messages that a user logs. Standard modifiers are provided for prepending a timestamp, thread ID, or append a newline to each message."
The word modifiers should link to modifier_logger.html.
Done.
Declaring/Defining Logs:
Change:
In order to be used, logs need to first be declared (and defined somewhere). You'll declare logs in a header file:
To:
Logs must be both declared and defined before they may be used. In general, users will declare logs in a header file:
Done.
modifier_logger.html:
"append an enter to the message, if not already found". Should be "newline", not "enter".
"prepend the log' name (example, if the log is named "app.charts.dbg", prepend this to the message)". Change to "prepend the log's name, for example "app.charts"". I think the use of "dbg" here is a remnant of the pre-levels design.
Done.
manipulating logs.html:
There are a number of cases in this section where the terminology gets muddled. Modifiers are called both modifiers and manipulators. Stick to a single name.
"Note that if modifier functions A and B have the *same *index, they way they get called is unspecified (you don't know for sure if A is called before B, or vice-versa)". Change "way they get called" to "order in which they are called" and drop the parenthesized phrase.
Done.
I like Daryl's proposed change to the way appenders are associated with logs. This might make sense for modifiers as well, and could make it quite easy to write a totally configuration-driven method to initialize the Boost.Log subsystem. However, doesn't this push thread-safety concerns into the appenders? For example, if several logs share a given appender, is it now up to the appender to serialize access to operator() instead of the log.
I don't think that would be a problem. The way I see it, only the interface changes.
- Are you knowledgeable about the problem domain?
Yes. I have used a number of logging APIs in a number of different languages, and implemented a couple myself.
I vote that this library be accepted into Boost pending some work on the documentation and after addressing some very minor implementation issues.
Thanks! 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!
On 11/8/05, John Torjo <john.lists@torjo.com> 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
Caleb Epstein wrote:
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.
Yes, you're right. Done it.
I just meant that there are older definitions left in the code, commented out, above the current definitions. Is this what you want?
Oh, right! Fixed it
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.
Yup, I'll fix it.
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.
I'm not sure how that would help. Could you give an example? Thanks. 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!
On 11/9/05, John Torjo <john.lists@torjo.com> wrote:
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.
I'm not sure how that would help. Could you give an example? Thanks.
Actually I'm not sure that changing the syntax of BOOST_LOG/LOGL would even be required, based on the way enabled_logger works. It would just involve enabled_logger holding a boost::shared_ptr<logger> and the log_name() function that is created by BOOST_DEFINE_LOG returning a shared_ptr and not a reference. -- Caleb Epstein caleb dot epstein at gmail dot com
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.
I'm not sure how that would help. Could you give an example? Thanks.
Actually I'm not sure that changing the syntax of BOOST_LOG/LOGL would even be required, based on the way enabled_logger works. It would just involve enabled_logger holding a boost::shared_ptr<logger> and the log_name() function that is created by BOOST_DEFINE_LOG returning a shared_ptr and not a reference.
I do have to give it some thought. 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!
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Hartmut Kaiser | Sent: 06 November 2005 20:02 | To: boost@lists.boost.org | Subject: [boost] [Review] The review of the Boost.Logging library starts today, November 7th 1 I have started to review this very promising looking library, but am deterred from trying it fully by the hassle of building the library. (I have yet to get bjam to work well for me, and 2 lines in the bjam file looks ominous <include>D:/boost/boost_latest/boost/ # FIXME remove this! as I use I:\boost_root\. I would persevere if I was confident I would use it - and I wasn't just going to upgrade for VS 8.0 beta2 to VS 8.0 ;-) I need: boost_log-vc80-mt-gd-1_33.lib and boost_log-vc80-mt-1_33.lib Could these be provided ready built for VS 7.1 and 8.0, 8.0 beta2 even? (For the convenience of the idle/incompetent - and until bjam/v2 and its documentation works foolproofly - 1.34?). (As promised in the documentation, building the sources each time is rather time-consuming to be practicable for real-life use). 2 I got several "warning C4996: 'localtime' was declared deprecated" which should be suppressed IMO, until a better global 'solution' has been adopted. # pragma warning(disable: 4996) // ' ' was declared deprecated. 3 Starting the simple example with the normal Ctrl F5 "start without debugging", the console window starts with: "CMD.EXE was started with '\\hetp3\i$\boost_root\libs\log\examples\simple\vc71 as the current directory path. UNC paths are not supported. <<<<<<<<<<<<<<<<<<<<<<<<< ?????????????????? Defaulting to Windows directory. <<< <<<<< I wasn't clear what this meant, so searched, finding out.txt (with sensible contents) in C:/winnt - slightly to my surprise! 15:10:11 [app] [Thread 2300] pre-start - this gets logged too! 15:10:11 [app.dbg] debug pre-start - this gets logged too! <snip> One the other hand using autostart as a post-build event (a MUCH more convenient method of starting console applications with Post build command line: "$(TargetDir)\$(TargetName).exe" --result_code=no --report_level=detailed --catch_system_errors=no and description Autorun "$(TargetDir)$(TargetName).exe" ------ Rebuild All started: Project: vc71, Configuration: Release Win32 ------ Deleting intermediate and output files for project 'vc71', configuration 'Release|Win32' Compiling... main.cpp log_manager.cpp log.cpp functions.cpp Generating Code... Linking... Embedding manifest... Autorun "\\hetp3\i$\boost_root\libs\log\examples\simple\vc71\release\vc71.exe" 15:43:06 [app] [Thread 1448] pre-start - this gets logged too! 15:43:06 [app.dbg] debug pre-start - this gets logged too! 15:43:06 [app] [Thread 1448] testing1-2-3 15:43:06 [app.dbg] this is a debug message, i=1 15:43:06 I just wanted to tell you something.... 15:43:06 [app.warn] Logged-on Users approach max. limit 15:43:06 [app.err] Too many users! 15:43:06 Creating main window 15:43:06 A debug msg coming from {charts} module 15:43:06 Destroying main window 15:43:06 [app] [Thread 1448] However, this msg. will 15:43:06 [app.dbg] this will be written - this log just got enabled 15:43:06 [app.err] this will be written - all logs are enabled 15:43:06 [app.dbg] this will be written 15:43:06 a simple info Build log was saved at "file://\\hetp3\i$\boost_root\libs\log\examples\simple\vc71\Release\BuildLog .htm" vc71 - 0 error(s), 0 warning(s) ========== Rebuild All: 1 succeeded, 0 failed, 0 skipped ========== and out.txt is at \\hetp3\i$\boost_root\libs\log\examples\simple\vc71\out.txt as expected. But overall looking good :-) Thanks. Paul -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
Paul A Bristow wrote:
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Hartmut Kaiser | Sent: 06 November 2005 20:02 | To: boost@lists.boost.org | Subject: [boost] [Review] The review of the Boost.Logging library starts today, November 7th
1 I have started to review this very promising looking library, but am deterred from trying it fully by the hassle of building the library.
(I have yet to get bjam to work well for me, and 2 lines in the bjam file looks ominous
<include>D:/boost/boost_latest/boost/ # FIXME remove this!
as I use I:\boost_root\.
I would persevere if I was confident I would use it - and I wasn't just going to upgrade for VS 8.0 beta2 to VS 8.0 ;-)
I need:
boost_log-vc80-mt-gd-1_33.lib
and
boost_log-vc80-mt-1_33.lib
I'm posting my 'bin' directory online: http://www.torjo.com/code/bin.zip It contains binaries for vc7.1 and vc8.0. I have a very old 8.0 version, so there could be problems on your machine.
Could these be provided ready built for VS 7.1 and 8.0, 8.0 beta2 even? (For the convenience of the idle/incompetent - and until bjam/v2 and its documentation works foolproofly - 1.34?).
(As promised in the documentation, building the sources each time is rather time-consuming to be practicable for real-life use).
It is time-consuming, but not that much -- for the purpose of the review. What would indeed be time-consuming, is making the library header-only. In addition to the posted bin.zip file, to build the log lib, you could do either of: - copy the library files into your BOOST_ROOT directory, or - get started by using one of the examples provided by the library. You could start with the 'basic_usage' example, or - include the source files into your projects - it's just 3 source files, and define the macro 'BOOST_LOG_NO_LIB' How does that sound?
2 I got several "warning C4996: 'localtime' was declared deprecated" which should be suppressed IMO, until a better global 'solution' has been adopted.
# pragma warning(disable: 4996) // ' ' was declared deprecated.
Thanks, added that.
[...] But overall looking good :-)
Thanks ;) 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!
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of John Torjo | Sent: 08 November 2005 21:14 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | library startstoday, November 7th | | Paul A Bristow wrote: | > | > | > | -----Original Message----- | > | From: boost-bounces@lists.boost.org | > | [mailto:boost-bounces@lists.boost.org] On Behalf Of Hartmut Kaiser | > | Sent: 06 November 2005 20:02 | > | To: boost@lists.boost.org | > | Subject: [boost] [Review] The review of the Boost.Logging | I'm posting my 'bin' directory online: | http://www.torjo.com/code/bin.zip Thanks, | It contains binaries for vc7.1 and vc8.0. I have a very old | 8.0 version, | so there could be problems on your machine. | | > | > Could these be provided ready built for VS 7.1 and 8.0, | > (For the convenience of the idle/incompetent Still useful IMO, and similarly for other libraries, considering the work involved for developers, it will encourage more people to use Boost. Building libraries is still a hurdle. | In addition to the posted bin.zip file, | to build the log lib, you could do either of: | - copy the library files into your BOOST_ROOT directory, but using the requested file to link (1_32??) doesn't work with my 1_33 and 8.0 beta2. Not unexpected :-( | - get started by using one of the examples provided by the library. | You could start with the 'basic_usage' example, or | - include the source files into your projects - | it's just 3 source files, log_manager.cpp log.cpp functions.cpp #define BOOST_LOG_NO_LIB sounds good ... BUT i:\boost_root\libs\log\src\functions.cpp(53) : error C2664: 'OutputDebugStringW' : cannot convert parameter 1 from 'const char *' to 'LPCWSTR' Adding a (LPCWSTR) makes this compile and link OK. But then re-compile your \examples\simple example fails to compile functions.cpp ..\..\..\src\functions.cpp(53) : error C2664: 'OutputDebugStringA' : cannot convert parameter 1 from 'LPCWSTR' to 'LPCSTR' Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast So I tried (LPCSTR) (quickest bodge) and with autorun works thus: Autorun "\\hetp3\i$\boost_root\libs\log\examples\simple\vc71\Release\vc71.exe" 17:41:18 [app] [Thread 2200] pre-start - this gets logged too! 17:41:18 [app.dbg] debug pre-start - this gets logged too! 17:41:18 [app] [Thread 2200] testing1-2-3 17:41:18 [app.dbg] this is a debug message, i=1 17:41:18 I just wanted to tell you something.... 17:41:18 [app.warn] Logged-on Users approach max. limit 17:41:18 [app.err] Too many users! 17:41:18 Creating main window 17:41:18 A debug msg coming from {charts} module 17:41:18 Destroying main window 17:41:18 [app] [Thread 2200] However, this msg. will 17:41:18 [app.dbg] this will be written - this log just got enabled 17:41:18 [app.err] this will be written - all logs are enabled 17:41:18 [app.dbg] this will be written 17:41:18 a simple info :-) Using CTL F5 to start a console window... CMD.EXE was started with '\\hetp3\i$\boost_root\libs\log\examples\simple\vc71' a s the current directory path. UNC paths are not supported. <<<<<<<<<<<<<<<<< Why not? Defaulting to Windows directory. 17:44:00 [app] [Thread 2188] pre-start - this gets logged too! 17:44:00 [app.dbg] debug pre-start - this gets logged too! 17:44:00 [app] [Thread 2188] testing1-2-3 17:44:00 [app.dbg] this is a debug message, i=1 17:44:00 I just wanted to tell you something.... 17:44:00 [app.warn] Logged-on Users approach max. limit 17:44:00 [app.err] Too many users! 17:44:00 Creating main window 17:44:00 A debug msg coming from {charts} module 17:44:00 Destroying main window 17:44:00 [app] [Thread 2188] However, this msg. will 17:44:00 [app.dbg] this will be written - this log just got enabled 17:44:00 [app.err] this will be written - all logs are enabled 17:44:00 [app.dbg] this will be written 17:44:00 a simple info Press any key to continue . . . And my out.txt file is in c:\winnt - somewhat to my surprise. I did quite a bit of fumbling to get the basic example working, starting right scratch as a complete novice. As ever, using macros, the error messages swiftly become inscrutable. Why would one want to create a file define.h just to hold one (or perhaps two) statements? It is just impossible to write documentation if you are the code author! (I find the greyed out code MOST irritating. - I don't see why one should want to change the background at all - it just decreases legibility. A very slight background is ample and MUCH better IMO - .code { width: 100%; border: none; background-color: #f0f0f0; } or #f0f0fa ? A list of macros and their function would be helpful. I have, for example, #define BOOST_LOG_WIN32 in init.cpp to activate #ifdef BOOST_LOG_WIN32 manipulate_logs("DEBUG").add_appender(write_to_dbg_wnd ); #endif and this works, writing to the console window. But the line manipulate_logs("app").add_appender(write_to_file("Test_log_out.txt") ); doesn't seem to create the file I expect, anywhere. I would expect Test_log_out.txt to be in project \debug or \release? Or in c:\winnt??? While I RTFM, perhaps you can advise on progress so far. Thanks Paul -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
| - get started by using one of the examples provided by the library. | You could start with the 'basic_usage' example, or
| - include the source files into your projects - | it's just 3 source files,
log_manager.cpp log.cpp functions.cpp
#define BOOST_LOG_NO_LIB
sounds good ...
BUT
i:\boost_root\libs\log\src\functions.cpp(53) : error C2664: 'OutputDebugStringW' : cannot convert parameter 1 from 'const char *' to 'LPCWSTR'
Adding a (LPCWSTR) makes this compile and link OK.
But then re-compile your \examples\simple example fails to compile
functions.cpp ..\..\..\src\functions.cpp(53) : error C2664: 'OutputDebugStringA' : cannot convert parameter 1 from 'LPCWSTR' to 'LPCSTR' Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
So I tried (LPCSTR) (quickest bodge) and with autorun works thus:
I've made it work for Unicode, and tested in on VC7.1: the basic_usage example now has a configuration called "Debug Unicode"
UNC paths are not supported. <<<<<<<<<<<<<<<<< Why not?
How should I know? :)
Defaulting to Windows directory. 17:44:00 [app] [Thread 2188] pre-start - this gets logged too! 17:44:00 [app.dbg] debug pre-start - this gets logged too! 17:44:00 [app] [Thread 2188] testing1-2-3 17:44:00 [app.dbg] this is a debug message, i=1 17:44:00 I just wanted to tell you something.... 17:44:00 [app.warn] Logged-on Users approach max. limit 17:44:00 [app.err] Too many users! 17:44:00 Creating main window 17:44:00 A debug msg coming from {charts} module 17:44:00 Destroying main window 17:44:00 [app] [Thread 2188] However, this msg. will 17:44:00 [app.dbg] this will be written - this log just got enabled 17:44:00 [app.err] this will be written - all logs are enabled 17:44:00 [app.dbg] this will be written 17:44:00 a simple info Press any key to continue . . .
And my out.txt file is in c:\winnt - somewhat to my surprise.
I would assume it's got something to do with the "Defaulting to Windows directory." message :)
I did quite a bit of fumbling to get the basic example working, starting right scratch as a complete novice.
As ever, using macros, the error messages swiftly become inscrutable.
Why would one want to create a file define.h just to hold one (or perhaps two) statements?
It was just an example ;)
(I find the greyed out code MOST irritating. - I don't see why one should want to change the background at all - it just decreases legibility. A very slight background is ample and MUCH better IMO - .code { width: 100%; border: none; background-color: #f0f0f0; } or #f0f0fa ?
Yes, sorry for that -- fixed.
A list of macros and their function would be helpful.
Noted, will do.
I have, for example,
#define BOOST_LOG_WIN32 in init.cpp to activate
For VC this is not needed.
#ifdef BOOST_LOG_WIN32 manipulate_logs("DEBUG").add_appender(write_to_dbg_wnd ); #endif
and this works, writing to the console window.
But the line
manipulate_logs("app").add_appender(write_to_file("Test_log_out.txt") );
doesn't seem to create the file I expect, anywhere.
I would expect Test_log_out.txt to be in project \debug or \release?
Or in c:\winnt???
I assume you have VC8.0, and I also assume that VC8.0 has (quite) a few bugs in it. Normally, "Test_log_out.txt" should be created in the project's Working directory, which by default should be the dir where the .sln file is found.
While I RTFM, perhaps you can advise on progress so far.
Lucky me I found out what RTFM means :) (from which, I had only guessed the F right ;)) Your progress is fine. I would suggest using VC7.1 if you have it, it's much more stable. By the way, are you using some command-line only compiler? 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!
John Torjo wrote:
While I RTFM, perhaps you can advise on progress so far.
Lucky me I found out what RTFM means :) (from which, I had only guessed the F right ;))
I'm pretty sure it stands for "Friendly" ;-) Regards, João
João Abecasis <jpabecasis@gmail.com> writes: | > Lucky me I found out what RTFM means :) | > (from which, I had only guessed the F right ;)) | | I'm pretty sure it stands for "Friendly" ;-) "Fine" is also often used. -- Lgb
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of João Abecasis | Sent: 10 November 2005 10:37 | To: Boost Devel | Subject: Re: [boost] [Review] The review of the Boost.Logging | library startstoday, November 7th | | John Torjo wrote: | >>While I RTFM, perhaps you can advise on progress so far. | > | > | > Lucky me I found out what RTFM means :) | > (from which, I had only guessed the F right ;)) | | I'm pretty sure it stands for "Friendly" ;-) In this case it certainly did ;-) Paul -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of John Torjo | Sent: 09 November 2005 21:02 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | library startstoday, November 7th | > | > So I tried (LPCSTR) (quickest bodge) and with autorun works thus: | | I've made it work for Unicode, and tested in on VC7.1: the | basic_usage | example now has a configuration called "Debug Unicode" Works OK in debug, but in release it tries to link to dll, despite #define Boost_LOG_NOLIB Perhaps by design? | I assume you have VC8.0, and I also assume that VC8.0 has (quite) a few | bugs in it. Normally, "Test_log_out.txt" should be created in the | project's Working directory, which by default should be the dir where | the .sln file is found. | | > While I RTFM, perhaps you can advise on progress so far. | | Lucky me I found out what RTFM means :) | (from which, I had only guessed the F right ;)) Documentation looks as good as one can expect when written by the code author - by definition, they are incapable of making an Really Fine job of this ;-) | Your progress is fine. I would suggest using VC7.1. I am just about to upgrade to the 8.0 release, and the time I have allotted to this review is consumed, but I persuded that this is the one to go for. Formal review to follow. Paul PS Using the VS IDE to create empty console application. -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
Paul A Bristow wrote:
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of John Torjo | Sent: 09 November 2005 21:02 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | library startstoday, November 7th | > | > So I tried (LPCSTR) (quickest bodge) and with autorun works thus: | | I've made it work for Unicode, and tested in on VC7.1: the | basic_usage | example now has a configuration called "Debug Unicode"
Works OK in debug, but in release it tries to link to dll, despite #define Boost_LOG_NOLIB
Perhaps by design?
It should be have been BOOST_LOG_NO_LIB Same result with this?
| I assume you have VC8.0, and I also assume that VC8.0 has (quite) a few | bugs in it. Normally, "Test_log_out.txt" should be created in the | project's Working directory, which by default should be the dir where | the .sln file is found. | | > While I RTFM, perhaps you can advise on progress so far. | | Lucky me I found out what RTFM means :) | (from which, I had only guessed the F right ;))
Documentation looks as good as one can expect when written by the code author - by definition, they are incapable of making an Really Fine job of this ;-)
Well, I need to do better then :) Thanks! 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!
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of John Torjo | Sent: 10 November 2005 15:29 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | library startstoday, November 7th | | It should be have been BOOST_LOG_NO_LIB | Same result with this? Yes seems so - and with 8.0 release. Tries to autolink? (Identical test code OK with debug). Paul -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
Paul A Bristow wrote:
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of John Torjo | Sent: 10 November 2005 15:29 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | library startstoday, November 7th | | It should be have been BOOST_LOG_NO_LIB | Same result with this?
Yes seems so - and with 8.0 release. Tries to autolink?
This is strange, because I just tried it myself (created a new Configuration - Release Unicode), and it works like a charm. I'll post it online in 1 hour or so. 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!
VS 2005 8.0 release. Microsoft Visual Studio 2005 Version 8.0.50727.42 (RTM.050727-4200) <<< Anyone know what the 4200 is? And is 2 * 8 = 16 why isn't version 16? _MSC_FULL_VER is 140050727 Test .\testLogging.cpp Thu Nov 10 18:07:32 2005 18:17:10 [myapp] testing99 18:17:10 [myapp] a simple info 18:17:10 [myapp] debug out99 Press any key to continue . . . MSVC++ compiler Version 8.0 log_manager.cpp i:\boost_root\boost/log/log.hpp(168) : warning C4512: 'boost::logging::logger_and_level' : assignment operator could not be generated i:\boost_root\boost/log/log.hpp(164) : see declaration of 'boost::logging::logger_and_level' i:\boost_root\libs\log\src\log_manager.cpp(83) : warning C4100: 'lvl' : unreferenced formal parameter You may wish so suppress these for MSVC? I presume they are spurious warnings? # pragma warning(disable: 4512) // assignment operator could not be generated. # pragma warning(disable: 4100) // unreferenced formal parameter. Paul | -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of John Torjo | Sent: 09 November 2005 21:02 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | library startstoday, November 7th
MSVC++ compiler Version 8.0 log_manager.cpp i:\boost_root\boost/log/log.hpp(168) : warning C4512: 'boost::logging::logger_and_level' : assignment operator could not be generated i:\boost_root\boost/log/log.hpp(164) : see declaration of 'boost::logging::logger_and_level'
i:\boost_root\libs\log\src\log_manager.cpp(83) : warning C4100: 'lvl' : unreferenced formal parameter
You may wish so suppress these for MSVC? I presume they are spurious warnings?
# pragma warning(disable: 4512) // assignment operator could not be generated. # pragma warning(disable: 4100) // unreferenced formal parameter.
Yup, you're right. Updating right now. 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!
today starts the review of the Boost.Logging library written by John Torjo. http://torjo.com/code/logging.zip
I took detailed look on the library and IMHO it still needs a lot of work. It would be better not to accept it as is now and wait until current problems get resolved. The overall concept feels sound and sufficient to me but the details should be more polished. What needs more work: * API: in [9] I suggest even more simple (but IMHO sufficient) interface to manipulate logs. Some other suggestions are speread throughout notes. Few new features are proposed. * Documentation. Overcommented examples are badly needed. Reference documentation is missing. Many details are not clear. See my comments bellow. * Efficiency. More attention could be devoted to make the tool fast. For too many people logging is hog that needs to be trimmed down the metal. See [25] for example. * Thread handling: I am not sure the flushing thread works as expected in EXE/DLL combinations [10]. The thread locking may be broken [5]. Once again I like the library but am annoyed by the not finished details. When finished the result will be both powerful and easy to use and will get widely used. /Pavel _____________________________________________________ 1. detail/defs.hpp: the #if (defined _WIN32) || .... could be replaced by: #if (defined BOOST_WINDOWS) && !(defined BOOST_DISABLE_WIN32) ------------------- 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. If the code is kept anyway then it should handle other compilers too, e.g. for BCB it is __DLL__. -------------------- The name BOOST_LOG_SOURCE is not very enlightening. It is also not mentioned in documentation. _____________________________________________________ 2. log_fwd.hpp: none of #include seems to be really needed here, just #include <string> #include <sstream> should be enough. _____________________________________________________ 3. log_manager.hpp: since this file is not intended for include by user it should be moved into detail/ directory. ------------------- Name DEFAULT_CACHE_LIMIT should be DefaultCacheLimit or so to avoid preprocessor clashes. ------------------- Since the functions here are in namespace "logging" the word "log" doesn't need to be repeated in many function names. _____________________________________________________ 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. -------------- The #include <windows.h> may be replaced by few simple forward defines and declaration to speed up the compilation. _____________________________________________________ 5. detail/ts_posix.hpp: the same advice related to Boost.Thread as above. -------------- assert() -->> BOOST_ASSERT() -------------- 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. _____________________________________________________ 6. basic_usage.html: the code snippets would look better if syntax highlighted. ------------- Code snippets for header and implementation file should be more visually separated. -------------- Code snippets may have more comments. Overcommenting is positive thing, especially for first few examples. -------------- Code snippet: functions add_appender() and add_modifier() sometimes take pointer and sometimes value. I guess this is mistake in docs. _____________________________________________________ 7. logs.html: there should be hyperlinks so one can check out what appenders/modifiers interfaces are. ---------------- Instead of "appender" word "target" may be better. Instead of "modifier" perhaps "formatter" sounds more common. ---------------- "much like the directory tree on an Operating System" ==>> "much like the directory tree on an filesystem" --------------- There should be mentioned whether "xyz*" matches "xyz" or not ------------------- It should be said log name is ASCIIZ. --------------------- The code snippet with BOOST_DEFINE_LOG(gui, "app.gui") should say what the 'gui' is - object, "string name" or "C++ name"? --------------------- 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. ----------------------- using namespace boost::logging; is missing in code snippets. In first code snippet you may use full qualification to give reader clue what namespaces are used where. _____________________________________________________ 8. modifier_logger.html: the header name #include <boost/log/functions.hpp> is very unintuitive. Perhaps "modifiers_and_appenders.hpp" would be better. --------------------- There should be table of all available modifiers and all available appenders at this moment. This table should contain one-line mini-examples. --------------------- An idea for another modifier: limit the text to gives maximal length. It may happend the dump is unexpectedly large and would fill disk/overwrite cache/take too much time/be unreadable. Limiting one log size would help here a lot and would relieve end user of doing it manually. ----------------------- Another idea for a modifier: limit length of line in dump and automatically insert newline after N characters. ----------------------- 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. ----------------------- Order in which appenders and modifiers are called is not specified. _____________________________________________________ 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." ------------------- DEFAULT_INDEX /cannot/ be uppercase (clashes with preprocessor). ------------------- 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. ---------------------- 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. ---------------------- In .add_modifier( func, index, name); it should be explained whatr is "name" and what is "index". It should be clear whether name has something in common with "C++ name" or "string name" and whether "index" relates to "level". ---------------------- Seeing term proliferation I suggest to add page "Term ditionary" and link the terms when used for the first time in docs and where it may be useful. -------------------- "insures" ==>> "ensures"? -------------------- typo "amongst" -------------------- 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. With such functionality you can get rid of del_modifier() API as well. Latest manipulate_logs() would define current state fully and would overwrite any previous settings. I very much suggest to use this quite simpler, more intuitive but sufficiently powerful way to manage logs. It is also easier than the Darryl Green's solution. -------------------- "default_" should be "default_level". Similarly other names. I suggest to get rid of "level" sub-namespace and use it as suffix. -------------------- Is it reasonable to use UINT_MAX for disable_all level? The value is 64 bits. Woudn't it be better to use: std::numeric_limits<unsigned>::max() ? ---------------------- 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. 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) } ---------------------------- You may obfuscate a little bit your email in docs to keep spambots out. _____________________________________________________ 10. predefined.html: append_enter ==>> append_newline ---------------- prepend_thread_id should exists also for POSIX (there's one simple function for 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" -------------------- write_to_dbg_wnd ==>> write_to_debug_window ts_appender ==>> longer, more intuitive name ---------------------- The docs for "ts_appender" suggests other ways are /not/ MT safe. This should be cleared out (I hope all ways are MT safe, no matter what I specify). The later code snippets do not clear my confusion. The other documentation for "ts_appender" is very confusing, I was not able to understand it. ----------------------- "appender_array" - confusing documentation, no idea what is says. ----------------------- rolling_file_appender - example needed, picture would help. Name should be "rotating....". ----------------------- periodic_file_appender - should have parameters to delete old log files (by time, by count or by size). Example is missing, docs is insufficient. The name should be changed. ------------------------- shared_memory_appender - insufficient docs, example missing. It should be explicitly said here there are no delays or caching when writing into shmem. Docs doesn't says what happend when shmem is full. I think the only reasonable behavior is to overwrite last logs in FIFO manner. Is it possible to use several shmems? Docs should say and should have couple of examples. -------------------- 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 would suggest to investigaet the problem and if real then to use Singleton library (or extract Jason's solution). If this is too much of work then the documentation should warn user that each executable (exe, dll, so) would have one separate thread and they cannot deal with the same file/shmem. ---------------------- Static destruction: currently the library cannot reliably log from destructors of static objects. The Jason's library contains support for this situation. -------------------------- 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. _____________________________________________________ 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 ); and escape sequences: $Xms - e.g. "12 ms", "78321 ms" (only milliseconds are used) $Xsec - e.g. "2 s, 23 ms" (only seconds and millis) $Xmin - e.g. "3 m, 21 s, 10 ms" $Xhour - e.g. "1 h, 33 m, 20 s, 54 ms" $Xday - e.g. "2 d, 1 h, 33 m, 20 s, 54 ms" This would allow automatically see and compare app performance characteristics and check timeouts. Different threads may use different time start points (e.g. useful for long lasting worker threads). _____________________________________________________ 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. _____________________________________________________ 13. Feature request: Boost.Filesystem should be supported (for specifying file names). The library is stable and likely in next Standard. _____________________________________________________ 14. efficiency.html: level is bigger ==>> level is higher -------------- 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. _____________________________________________________ 15. caching.html: the docs doesn't say whether flushed logs follow the rules given by manipulate_logs(). -------------- It doesn't say whether manual flush() is needed and what is default. Complete example is sorely needed. -------------- The docs doesn't say what int cache_limit = 256 means. What is 256? Bytes, logs, lines? -------------- I do not like very much functions as flush_log_cache() are standalone functions. I would prefere use of singleton or monostate pattern here, just for better feeling. _____________________________________________________ 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. _____________________________________________________ 17. scoped.html: cannot BOOST_SCOPEDLOG be somehow merged with BOOST_LOG? Having two macros is confusing and error prone. Perhaps some Boost.Preprocessor trick could be employed here. _____________________________________________________ 18. compile_time_logs.html: the fact that "level" is ignored is strange and counterintuitive. If implementation cannot be changed here the docs should stress this strange behaviour more. Perhaps my suggestion in [14] (about completely compiling out any logging support) could be integrated with "compile time logging". ---------------- The term "compile time logs" is misleading, "compile time log enabling" or so would be better. -------------- The example should show mix of runtime and compile time loggers working together. _____________________________________________________ 19. examples.html: every example could be linked here, together with short description. _____________________________________________________ 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. _____________________________________________________ 21. view.html: several examples of real output may be here, from simplest to most complicated ones. _____________________________________________________ 22. Source files: it may be considered to use only one *.cpp file to simplify needed changes in makefiles. _____________________________________________________ 23. The documentation may contain more tables, listing all available options for features + tiny examples. -------------------- The documentation lacks reference for some important objects like manipulate_logs(). At least list of all standalone functions should exists. -------------------- The documentation may contains some short overview of internal structure of the library. This is just nice to have, not absolute requirement. -------------------- The documentation doesn't say whether and exception can be throwns out of it and what happens in such case. It is possible to play a lot increasing exception safety (for example to use internally allocator that may fall back to static memory block if bad_alloc). This is /a lot/ of work to do it completely and right, though. The documentation should at least warn that in low memory situation results are not predictable. -------------------- Exceptions thrown from the library (e.g. failure to create lazy thread for flushing) should be documented and also be present in log_fwd.hpp. -------------------- The documentation should contain complete example how to write a) custom appender and modifier b) custom back end (e.g. my own shmem). This one is very important. -------------------- Nice to have docs wish: a page discussing stranghts of this logger vs other common ones (log4cpp etc). -------------------- Documentation about performance of the library is missing - absolute times, # of dynamic allocations per typical log, etc. -------------------- Docs should have table of all macros (visible to user) and info whether they can be defined by user. For example now BOOST_LOG_NO_LIB is not mentioned anywhere and it is unclear what it is for. _____________________________________________________ 24. Boost license should be used. _____________________________________________________ 25. src/functions.cpp: function write_to_dbg_wnd() should be specialised for char and for wchar_t and OutputDebugStringA() and OutputDebugStringW() used respectively in each specialisation. -------------------- Functions like prepend_prefix() may use some preallocated buffer to write their output. Just this function does 3 dynamic allocations and 3-4 copyings of complete log data. I have seen people doing a lot of work to make logs as fast as possible, even writing their own printf()s or delaying their evaluation, fiddling with stack etc. 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. --------------------- Docs saying that thread ID is available only in Win32 is wrong - I see POSIX implemented. -------------------- It may be worth to measure whether GetCurrentThreadId() is fast enough or whether thread local variable storing the ID is faster. (No guess from me.) ---------------- prepend_time::prepend_time() array indexes; may use reserve() to avoid needless reallocations. Even static array would be far then enough for the few sequences. The whole code feels underoptimized to me. ---------------- prepend_time_strf(): this function does two system calls (time() and localtime()). Perhaps separate thread may be considered. This thread would sleep for cca 1 second then it would find local time and save it into certain data structures. prepend_time_strf() would simply fetch in these data structures w/o any system calls. This may have positive effect inside tight loops of performace sensitive apps. If milliseconds are needed (and implemented) then prepend_time_strf() needs to use different algorithm (but still single system call should be enough). _____________________________________________________ 26. Similary to OutputDebugString() on Win32 with POSIX saving to syslog may be implemented. On Win32 EventLog suppot may be added (very handy for errors). _____________________________________________________ 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. _____________________________________________________ 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. ------------ "searches" arrar may be reserve()d before. --------------- Does the matching works with pattern like: "abc.*.xyz" ? The docs doesn't say. ---------------- The code may be modified to deal with exception support switched off. File <boost/detail/no_exceptions_support.hpp> contains few helpers. --------------- 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. _____________________________________________________ 29. log.hpp: class logger: the member m_destroyed looks like having no real effect at all. _____________________________________________________ 30. Compiling examples with BCB 6.4: functions.hpp needs: #include <ctime> and in "struct prepend_time_strf": time_t m_t; tm m_tm; ==>> std::time_t m_t; std::tm m_tm; ----------------------- 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 The same wrapping should be in log/functions.hpp. File src/log_manager.cpp should have #if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564)) # pragma warn -8026 // some functions are not expanded inline #endif on top ----------------------- Files: * src/functions.cpp * examples/fast_compile/manipulate_logs.cpp: sprintf ==>> std::sprintf _____________________________________________________ 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 _____________________________________________________ 32. the example examples/shmem_example/writer/writer.cpp should also clean up the shmem memory when the data are read out sucessfully. Issue of MT safety: you write two different items: "shared_log_object" and "shared_occupied_size". This is MT unsafe though unlikely in practice. I would recommend to overhaul the shmem completely since current implementation is not very practical. Better implementation would allow: * reading and writing items into shmem in paralel, at any time and repeatedly. * overwriting oldest logs if shmem is full. _____________________________________________________ EOF
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!
"John Torjo" wrote:
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?
Probability of "DEFAULT_INDEX" being macro defined in end user code is very high. _____________________________________________________
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?
Jason's library allows to have singleton compiled into static library linked into a DLL whcih is loaded dynamically during session by EXE. And to do it portably. He has current code in Sandbox (documentation is being worked on but he expresses wish to explain what's needed). Reuse of Singleton library (quite likely to get into Boost in its second review) would allow to integrate Logger into wider mesh of singletons _____________________________________________________
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.
I would like to process result logs with my SW. I am lazy and would like to have clear markers of log start and end so the SW is easier. E.g. <<< 2005/11/24 12:38:43 blah blah blah
<<< 2005/11/24 12:38:56 ble ble
/Pavel
Pavel Vozenilek wrote:
"John Torjo" wrote:
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?
Probability of "DEFAULT_INDEX" being macro defined in end user code is very high.
I wouldn't call this very high - since usually you can define it as a constant. But anyway, I get the point ;)
Jason's library allows to have singleton compiled into static library linked into a DLL whcih is loaded dynamically during session by EXE. And to do it portably.
He has current code in Sandbox (documentation is being worked on but he expresses wish to explain what's needed).
Reuse of Singleton library (quite likely to get into Boost in its second review) would allow to integrate Logger into wider mesh of singletons
I don't have a problem with using his lib, once it gets into Boost.
I would like to process result logs with my SW.
I am lazy and would like to have clear markers of log start and end so the SW is easier.
What's SW? 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!
"John Torjo"
I would like to process result logs with my SW.
I am lazy and would like to have clear markers of log start and end so the SW is easier.
What's SW?
software /Pavel
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Hartmut Kaiser | Sent: 06 November 2005 20:02 | To: boost@lists.boost.org | Subject: [boost] [Review] The review of the Boost.Logging | | Please always state in your review, whether you think the library should be | accepted as a Boost library! Yes - definitely. I think Boost badly needs a logging system and this is a the best basis on which to start. Unlike many libraries, logging is highly environment sensitive and not easily tested using formal tests like Boost library: it needs an 'assault' by many and varied users. Like the Boost Test library when first accepted, this has some rough edges, but it is only by accepting it that we will get the feedback from users and give an the incentive to the author to refine the library. | Additionally please consider giving feedback on the following general topics: | | - What is your evaluation of the design? Looks sufficiently flexible for most purposes and reasonably efficient. I see no 'killer' criticisms of the design from domain experts. | - What is your evaluation of the implementation? Not qualified to judge, but it needs more refinement, particularly on the very many different platforms and mode of use. It needs more varied real-life use. I'd very much like to see an optional interface with the Boost date-time library. | - What is your evaluation of the documentation? Looks reasonable, but needs refinement in the light of user experience. Layout is plain and simple and fine for me (I doubt the work involved in producing fancier versions is worth it). I may be willing to assist with 'editorial' work on it. | - What is your evaluation of the potential usefulness of the library? Very useful. | - Did you try to use the library? Yes. Built some of the provded examples OK. I didn't try to build the library but added the three files to the project. Used the VS IDE to create empty console application. With what compiler? VS 8.0 beta 2. Did you have any problems? Yes, several minor. | - How much effort did you put into your evaluation? About 2 hours. | - Are you knowledgeable about the problem domain? No. A naive would-be user.
Paul A Bristow wrote:
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Hartmut Kaiser | Sent: 06 November 2005 20:02 | To: boost@lists.boost.org | Subject: [boost] [Review] The review of the Boost.Logging | | Please always state in your review, whether you think the library should be | accepted as a Boost library!
Yes - definitely. I think Boost badly needs a logging system and this is a
Thanks!
I'd very much like to see an optional interface with the Boost date-time library.
It's on my TODO list ;)
| - What is your evaluation of the documentation? Looks reasonable, but needs refinement in the light of user experience. Layout is plain and simple and fine for me (I doubt the work involved in producing fancier versions is worth it). I may be willing to assist with 'editorial' work on it.
Super! I'll call you on that :) Thanks. 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!
On 11/6/05, Hartmut Kaiser <hartmut.kaiser@gmail.com> wrote:
Hi all,
today starts the review of the Boost.Logging library written by John Torjo. It will end on November 16th.
[snip snip] Before I start, I'm very happy that this library is being considered for inclusion in Boost - I've been using it for some time now, and overall I have been very happy with it. - What is your evaluation of the design? I like the design. The concepts are very well thought out. My only stumbling block has been that the default log manager can only be extended in an all-or-nothing fashion, which makes extending the logging solution quite difficult. I would really like to have the option to simply replace the stream type in the default log manager, instead of the whole thing. This would make structured logging trivial. - What is your evaluation of the implementation? There are still a few rough edges which need to be ironed out. Specifically, there are a lot of unused parameters and signed/unsigned mismatches which make compilation with gcc a bit too noisy for my liking. There are quite a few TOTHINK etc. comments in the code, which suggests that the author isn't quite happy with the implementation details yet. These are, fortunately for me, mostly in areas which I do not use. - What is your evaluation of the documentation? The documentation lacks any rationale for the decisions made, and while it provides sufficient information for somebody who wants to use the default facilities (perhaps with a custom appender or two), I feel that the documentation could be improved by: - a high level, conceptual, overview of the library and the facilities it provides - adding rationale for design decisions - more information on extending the library, possibly with examples - I found myself guessing far too much. I also felt that more advanced examples (and tests!) would have been useful when starting out with the library. - What is your evaluation of the potential usefulness of the library? Extremely useful - I've already integrated it into one of our systems at work! - Did you try to use the library? With what compiler? Did you have any problems? As above, yes. I've used the library with vc71, Apple GCC 4, HP aCC 0.3.55 and Sun Studio 10 CC. I had to modify the source lightly to get rid of warnings, remove the use of boost::function (so that aCC, but a slightly older version, has a chance) and remove the DLL bits so that I wouldn't have to jump through too many hoops when moving the code into my source tree. As stated above, I've only built the library outside of Boost.Build, but I found the code and structure easy to understand and modify. If the library is accepted into Boost, and if aCC gets more capable, I would naturally use the Boost version of the code, not my own in-tree version. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I've used the library extensively over the last three months. - Are you knowledgeable about the problem domain? Only insofar as I am aware of the problems of our existing logging systems at work, and how this solution addresses those areas. I feel that this library should be accepted into Boost, but that attention should be paid to the areas above (particularly the documentation for extenders). Regards, Michael
Before I start, I'm very happy that this library is being considered for inclusion in Boost - I've been using it for some time now, and overall I have been very happy with it.
Thanks ;)
- What is your evaluation of the design?
I like the design. The concepts are very well thought out.
My only stumbling block has been that the default log manager can only be extended in an all-or-nothing fashion, which makes extending the logging solution quite difficult.
I would really like to have the option to simply replace the stream type in the default log manager, instead of the whole thing. This would make structured logging trivial.
Added to my TODO list :)
- What is your evaluation of the documentation?
The documentation lacks any rationale for the decisions made, and while it provides sufficient information for somebody who wants to use the default facilities (perhaps with a custom appender or two), I feel that the documentation could be improved by:
- a high level, conceptual, overview of the library and the facilities it provides - adding rationale for design decisions - more information on extending the library, possibly with examples - I found myself guessing far too much.
Yes, after the review, I want to do the docs in doxygen.
I also felt that more advanced examples (and tests!) would have been useful when starting out with the library.
Noted.
- Are you knowledgeable about the problem domain?
Only insofar as I am aware of the problems of our existing logging systems at work, and how this solution addresses those areas.
I feel that this library should be accepted into Boost, but that attention should be paid to the areas above (particularly the documentation for extenders).
Thanks, and will do! 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!
Hi all, I'm not making a full review, just a feature request, since I'm not having enough time lately to even update to the latest version of the library. But I'm using this library for sometime now(a few months) and it is working okay for me. The feature request I would like to make is: In the version I'm working(and probably the newer one either), it is possible to add appenders with functors/functions with two string parameters. This is a very useful thing since it let the user extend the library to log in differents storages/streams/etc. However, the library assumes that the function it calls operates in a real stream, and buffers based on this assumption, restricting the usefullness and the reuse of the log interface for use on "message-based" storages. The real feature request is that it be possible to tune the library to be able to work in a message-based form, using flush commands (like std::endl) to indicate the end of the message. My use-case for this feature is writing in the Event Logger in Windows 2000/2003/XP. I sent this feature request to John Torjo sometime ago, but didnt got any reply, so I thought in reposting it in the review process. So that, perhaps, more people may be having this need too. If this is already covered in the new version, then I'm sorry for wasting your time. For the rest of the library, tt is very good and, IMO, it should be accepted in boost. Thanks, -- Felipe Magno de Almeida Developer from synergy and Computer Science student from State University of Campinas(UNICAMP). Unicamp: http://www.ic.unicamp.br Synergy: http://www.synergy.com.br "There is no dark side of the moon really. Matter of fact it's all dark."
Hi Felipe, First of all sorry for not answering to you sooner.
The feature request I would like to make is:
In the version I'm working(and probably the newer one either), it is possible to add appenders with functors/functions with two string parameters. This is a very useful thing since it let the user extend the library to log in differents storages/streams/etc. However, the library assumes that the function it calls operates in a real stream, and buffers based on this assumption, restricting the usefullness and the reuse of the log interface for use on "message-based" storages. The real feature request is that it be possible to tune the library to be able to work in a message-based form, using flush commands (like std::endl) to indicate the end of the message.
I'm not sure I fully understand what you're requesting. Could you give an example?
For the rest of the library, tt is very good and, IMO, it should be accepted in boost.
Thanks! 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!
I have a couple of design questions: 1. What is the fundamental reason that logs cannot be declared/defined with something as simple as boost::logging::log mylog ("mylog"); ? What special needs do you have that rule out this approach? (I took a brief look at the source code, got a vague idea what you were trying to accomplish, and have some ideas about how the same may be accomplished without resorting to macro's, but would like to hear your explanation and reasoning first.) 2. I am very skeptical about logs having both a C++ identifier and a string name. The documentation argues that the string names bring "extreme flexibility" in the following two ways: a: External identification While I agree that external identification may sometimes be useful to some users, is it really necessary to integrate it into the library? Would the users who require such functionality not be able to simply use something like a std::map<std::string, log *> ? b: Log hierarchies I do not question the usefulness of log hierarchies, but am not convinced that string names (and wildcard queries) are the best way to implement them. It seems to me that all that is needed to be able to form hierarchies of logs, is a way to express parent->child relationships between two logs. Indeed, something as simple as parent_log.append_child(child_log); should suffice. Wildcarded queries would simply become logs (or some sort of log-proxies) by themselves, that the user must manually connect to the desired sink logs. While this approach is slightly more cumbersome, I feel that getting rid of all the string name cruft is definitely a big win. What are your feelings about this? Regards, Eelis
Eelis van der Weegen wrote:
I have a couple of design questions:
1. What is the fundamental reason that logs cannot be declared/defined with something as simple as boost::logging::log mylog ("mylog"); ? What special needs do you have that rule out this approach? (I took a brief look at the source code, got a vague idea what you were trying to accomplish, and have some ideas about how the same may be accomplished without resorting to macro's, but would like to hear your explanation and reasoning first.)
Note: you can actually do the above: it's called "Scoped Logs" - look it up in the docs: logger gui("app.gui"); BOOST_SCOPEDLOG(gui) << "something GUIsh" << std::endl;
2. I am very skeptical about logs having both a C++ identifier and a string name. The documentation argues that the string names bring "extreme flexibility" in the following two ways:
a: External identification
While I agree that external identification may sometimes be useful to some users, is it really necessary to integrate it into the library? Would the users who require such functionality not be able to simply use something like a std::map<std::string, log *> ?
In the simplest of cases (forgetting the thread-safety issues), perhaps that would be enough. In this case, you would then have to provide methods to add/del appenders/modifiers, and add/del them for each log -- which I consider troublesome. See Log Hierarchies below
b: Log hierarchies
I do not question the usefulness of log hierarchies, but am not convinced that string names (and wildcard queries) are the best way to implement them.
It seems to me that all that is needed to be able to form hierarchies of logs, is a way to express parent->child relationships between two logs. Indeed, something as simple as parent_log.append_child(child_log); should suffice.
Wildcarded queries would simply become logs (or some sort of log-proxies) by themselves, that the user must manually connect to the desired sink logs.
While this approach is slightly more cumbersome, I feel that getting rid of all the string name cruft is definitely a big win.
I feel that using strings for log hierarchies provides for very loose coupling. More to the point, you can have multiple modules (or DLLs, for that matter) that need to do logging. You can externally identify such a log by a string, and somewhere else, specify its appenders/modifiers. Therefore, even if you have multiple DLLs, each of which does logging, you can manipulate their appenders/modifiers in the main application module. Say I want to create this log: "charts.gui". If you were to use your approach, lets say you would first get to the root log, like this: get_root_log(). But how do I get to the charts log? More or less, I'd end up using some string again: get_root_log()->child("charts"); Or, how would you go about enumerating your logs? Once you get to a reference to a log, what can you do with it? It would be just a mere pointer... Also, I believe that if you're not using strings, in time you can lose the big picture: hey, X is a child of Y, which is a child of Z, or it is T, or W? Finally, in a large application, you could end up with different modules that have, lets say, a special 'gui' log. If so, you can give each such log the "gui" suffix. This way, you can manipulate all gui logs like this: "*.gui.*" For instance, to write all the GUIsh information to the debug window, you'd say: manipulate_logs("*.gui.*").add_appender(write_to_dbg_wnd); 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!
John Torjo wrote:
Eelis van der Weegen wrote:
1. What is the fundamental reason that logs cannot be declared/defined with something as simple as boost::logging::log mylog ("mylog");
Note: you can actually do the above: it's called "Scoped Logs"
The existence of scoped logs doesn't really affect my question: Assuming that BOOST_DECLARE_LOG/BOOST_DEFINE_LOG have a good reason to exist (for example because logger objects cannot be used at namespace level), what is the fundamental reason that they have to be macro's? Why can't we get the normal non-macro syntax at namespace level without losing functionality? Regarding string names -- while I do not dispute that some users may have a use for a runtime object identification system based on string names or other runtime identification values, I am convinced that such systems solve a completely different problem that in its core has nothing to do with logging. I therefore feel that a logging library should just work with normal C++ identifiers, variables, references, iterators, etc. Any nifty string mappings (and naming/query schemes) can always be built on top of that if needed. That said, here's my review: - What is your evaluation of the design? See above. I feel that one of the library's primary features is misguided, and feel that the focus of the library is wrong. - What is your evaluation of the implementation? Haven't looked. - What is your evaluation of the documentation? Sloppy and unprofessional. Just not up to Boost standards. - What is your evaluation of the potential usefulness of the library? I have too little experience with logging in general to say. - Did you try to use the library? With what compiler? Did you have any problems? Haven't tried. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I read the documentation. - Are you knowledgeable about the problem domain? Not particularly. My review shouldn't carry much weight. - Do you think the library should be accepted as a Boost library? No. Regards, Eelis
Eelis van der Weegen wrote:
John Torjo wrote:
Eelis van der Weegen wrote:
1. What is the fundamental reason that logs cannot be declared/defined with something as simple as boost::logging::log mylog ("mylog");
Note: you can actually do the above: it's called "Scoped Logs"
The existence of scoped logs doesn't really affect my question: Assuming that BOOST_DECLARE_LOG/BOOST_DEFINE_LOG have a good reason to exist (for example because logger objects cannot be used at namespace level), what is the fundamental reason that they have to be macro's? Why can't we get the normal non-macro syntax at namespace level without losing functionality?
It's because the implementation of BOOST_DECLARE/DEFINE_LOG might change in time. Right now, it's a function returning a static reference to a logger. However, in time, if I find something better, it will change.
Regarding string names -- while I do not dispute that some users may have a use for a runtime object identification system based on string names or other runtime identification values, I am convinced that such systems solve a completely different problem that in its core has nothing to do with logging. I therefore feel that a logging library should just work with normal C++ identifiers, variables, references, iterators, etc. Any nifty string mappings (and naming/query schemes) can always be built on top of that if needed.
There's nothing stopping from not using the string names, if you really don't wish it. 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!
-----------------------------------------------------------------
- What is your evaluation of the design?
Good foundation, lots of features, but a bit complex and opaque. Some main thoughts: 1) It might be helpful if loggers and managers were "promoted" to objects with an obvious interface that can be easily passed around. 1.1) I haven't had a problem passing loggers around by use using a logger& or a logger*, but the interface seems to assume that generally the code will be compiled where the log name is known. Most of the code I work on is header or dll code that needs to be passed a logger by the main system. 1.2) I think the whole design would be easier to "drop in" if it were provided with a "public" manager interface and the manager objects could be easily passed around. 1.2.1) I think the static manager makes the design harder to use and control in a system that isn't a single executeable. This could be solved by either forcing the user to declare the manager, or possibly by leaving it static, but making all the functions take an explicit manager ref or pointer, or making the functions members of the manager. 1.2.1.1) The static manager seems particularly problematic for dlls. 1.2.1.1.1) Currently, the dll can't call the api functions and act on the application's logs, since the calls will be compiled to act on the dll's static log manager (I think). Although "externally visible" is a design premise of the library, I don't think the dll can even enable or disable application logs it knows the name for because the call will go to its own logs of the same name. Similarly the application can't control the dlls logs for the same reason... I think!... maybe I missed how to do this. If that's the case maybe the doc just needs to be more extensive. 1.2.1.1.2) It looks like there could actually be multi-threaded issues writing to logs from a dll, because, if the log thread is used, both the dll and the app will start their own trhead to write to the log. Presumably the log is ultimately a single object, like the screen or a file. I suppose if the locking uses a "named" lock that the OS api can look up this could be solved. However, it looks like on windows at least a Critical Section is used, which I don't think is named, so I don't think the one or more dlls running the thread and the application would actually lock each other. It seems like they could step on each other. 1.3) I can't seem to pass around log levels. I think this is because they are const ints, not members of an enum. This means the following code won't compile: void SomeClass::Dump(boost::logging::level_type eLevel, boost::logging::logger& rLog) { BOOST_SCOPEDLOGL(rLog, eLevel) << "Some Msg" << m_iSomeMemberVariable << endl; } This is kind of important in our code. 2) BOOST_LOG and BOOST_SCOPEDLOG are way too many uppercase characters to type. :) Ok, so that one's kind of petty. But seriously, as a veteran of many many many printfs, my wrists cringe when my brain tells them to type that out. Seriously again, I'm not sure this is an easy problem to solve. It's hard to come up with a meaningful short name that doens't clash with everything else. The best solution might be for the programmer to make an macro in their IDE that print "BOOST_LOG() <<" when they press ctrl+alt+b or something. If the name clash issue could be solved the boost log libary could provide some reasonable default. I added the following macros to my code, which I like, but I'm not sure are fit for a library: #define LG(log) BOOST_LOG(log) #define LGPTR(pLog) BOOST_SCOPEDLOG(*pLog) #define LGM() LGPTR(m_pLog) //Log to member #define LGP() if(pLog)LGPTR(pLog) //Log to param Then this code works: void SomeFunction(logger* pLog) { LGP() << "xxx"; } or this class x { logger* m_pLog; void AMemberFunc(){ LGM() << "xxx"; } }; -----------------------------------------------------------------
- What is your evaluation of the implementation?
Looks fine. No usage headaches. Find a few minor bugs, to be expected. No major ones. -----------------------------------------------------------------
- What is your evaluation of the documentation?
Good start, but as usual, could use more - John we know you need more work :). In particular, I think some description of the underlying implementation would be helpful. I'd like to see a description of the major objects and their data structures, and their relation to each other. I'd like to see a description of how "add_appender" works. It took me a while to struggle through this in the code. In particular, I'd like to see a description of how exactly the BOOST_LOG macro works! This is the most important part of the libary, and there are couple indirections an inversion in the implementation, (which seems fine), but takes a bit to understand. John, I can volunteer some hours in next couple weeks to help review or add doc if you like. If that would be helpful, I just need some direction files to review or subject matter. You can e-mail me at "a dot schweitzer dot grps at gmail dot com" -----------------------------------------------------------------
- What is your evaluation of the potential usefulness of the library?
- Did you try to use the library? With what compiler? Did you have any
Very useful. ----------------------------------------------------------------- problems? I've been using the library at home with VC7.1 We can't currently use it at work as we have a compiler that doesn't support nice things like boost function. No serious problems. Seems to be working pretty well. I've spent 4 - 8 hours a week over last several weeks debugging a multi-threaded program with it, and more hours a couple months ago testing out its various functions. -----------------------------------------------------------------
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A quick reading, some usage, then about 8 hours of reading over last couple months to understand various aspects. -----------------------------------------------------------------
- Are you knowledgeable about the problem domain?
More or less. Spent last several years programming an embedded system with no debugger or emulator but with a serial port. Printf is my good friend. Have spent a some time writing (and putting bugs in) our own implementation, which is not nearly so full featured. Andrew
Hi Andrew,
Some main thoughts: 1) It might be helpful if loggers and managers were "promoted" to objects with an obvious interface that can be easily passed around.
Managers will be promoted to objects. Perhaps loggers can be promoted too.
1.1) I haven't had a problem passing loggers around by use using a logger& or a logger*, but the interface seems to assume that generally the code will be compiled where the log name is known. Most of the code I work on is header or dll code that needs to be passed a logger by the main system.
Not sure I follow...
1.2.1) I think the static manager makes the design harder to use and control in a system that isn't a single executeable. This could be solved by either forcing the user to declare the manager, or possibly by leaving it static, but making all the functions take an explicit manager ref or pointer, or making the functions members of the manager.
If you take a look at log_manager.hpp, you'll see that this is how it happens now. Or have I misunderstood something?
1.2.1.1) The static manager seems particularly problematic for dlls.
1.2.1.1.1) Currently, the dll can't call the api functions and act on the application's logs, since the calls will be compiled to act on the dll's static log manager (I think). Although "externally visible" is a design premise of the library, I don't think the dll can even enable or disable application logs it knows the name for because the call will go to its own logs of the same name. Similarly the application can't control the dlls logs for the same reason... I think!... maybe I missed how to do this. If that's the case maybe the doc just needs to be more extensive.
The docs need to be more extensive on this issue ;)
1.2.1.1.2) It looks like there could actually be multi-threaded issues writing to logs from a dll, because, if the log thread is used, both the dll and the app will start their own trhead to write to the log. Presumably the log is ultimately a single object, like the screen or a file. I suppose if the locking uses a "named" lock that the OS api can look up this could be solved. However, it looks like on windows at least a Critical Section is used, which I don't think is named, so I don't think the one or more dlls running the thread and the application would actually lock each other. It seems like they could step on each other.
If you specify the appenders/modifiers only in one place (like, in the Executable), there are no problems.
1.3) I can't seem to pass around log levels. I think this is because they are const ints, not members of an enum. This means the following code won't compile:
void SomeClass::Dump(boost::logging::level_type eLevel, boost::logging::logger& rLog) { BOOST_SCOPEDLOGL(rLog, eLevel) << "Some Msg" << m_iSomeMemberVariable << endl; }
At this time, indeed you can't. I'll find a solution. In particular, log levels can't be members of an enum, since you should be able to add your own at any time.
2) BOOST_LOG and BOOST_SCOPEDLOG are way too many uppercase characters to type. :)
Well, you're right :) But so far can't think of any names that could be shorter...
-----------------------------------------------------------------
- What is your evaluation of the implementation?
Looks fine. No usage headaches. Find a few minor bugs, to be expected. No major ones.
-----------------------------------------------------------------
- What is your evaluation of the documentation?
Good start, but as usual, could use more - John we know you need more work :). In particular, I think some description of the underlying implementation would be helpful. I'd like to see a description of the major objects and their data structures, and their relation to each other. I'd like to see a description of how "add_appender" works. It took me a while to struggle through this in the code.
In particular, I'd like to see a description of how exactly the BOOST_LOG macro works! This is the most important part of the libary, and there are couple indirections an inversion in the implementation, (which seems fine), but takes a bit to understand.
John, I can volunteer some hours in next couple weeks to help review or add doc if you like. If that would be helpful, I just need some direction files to review or subject matter. You can e-mail me at "a dot schweitzer dot grps at gmail dot com"
I'll definitely call you on that! Andrew, was this a formal review? I'm asking, since I did not see the "Yes" (accept) or "No" (reject) at the end of it. 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!
John Torjo wrote:
Hi Andrew,
[snip]
1.1) I haven't had a problem passing loggers around by use using a logger& or a logger*, but the interface seems to assume that generally the code will be compiled where the log name is known. Most of the code I work on is header or dll code that needs to be passed a logger by the main system.
Not sure I follow...
I meant there's no technical problem passing loggers around, it just seems from the doc it wasn't assumed that would happen... so maybe there could end up being problems. None seen so far.
1.2.1) I think the static manager makes the design harder to use and control in a system that isn't a single executeable. This could be solved by either forcing the user to declare the manager, or possibly by leaving it static, but making all the functions take an explicit manager ref or pointer, or making the functions members of the manager.
If you take a look at log_manager.hpp, you'll see that this is how it happens now. Or have I misunderstood something?
I'll look again. [snip]
1.2.1.1.2) It looks like there could actually be multi-threaded issues writing to logs from a dll, because, if the log thread is used, both the dll and the app will start their own trhead to write to the log. Presumably the log is ultimately a single object, like the screen or a file. I suppose if the locking uses a "named" lock that the OS api can look up this could be solved. However, it looks like on windows at least a Critical Section is used, which I don't think is named, so I don't think the one or more dlls running the thread and the application would actually lock each other. It seems like they could step on each other.
If you specify the appenders/modifiers only in one place (like, in the Executable), there are no problems.
But then the dll can't ask the app for it's logs and attach appenders / modifiers too them.
1.3) I can't seem to pass around log levels. I think this is because they are const ints, not members of an enum. This means the following code won't compile:
void SomeClass::Dump(boost::logging::level_type eLevel, boost::logging::logger& rLog) { BOOST_SCOPEDLOGL(rLog, eLevel) << "Some Msg" << m_iSomeMemberVariable << endl; }
At this time, indeed you can't. I'll find a solution.
In particular, log levels can't be members of an enum, since you should be able to add your own at any time.
Maybe just a good old int.
2) BOOST_LOG and BOOST_SCOPEDLOG are way too many uppercase characters to type. :)
Well, you're right :) But so far can't think of any names that could be shorter...
Yes, maybe we need some parallel processing on that one. Maybe someone out there has a good idea.
Andrew, was this a formal review? I'm asking, since I did not see the "Yes" (accept) or "No" (reject) at the end of it.
Yes. I have some reservations, described in this e-mail. The most important is probably allowing managers to be more easily controlled, and adding more doc of the core of the interface. But it seems to work, and the perfect can be the enemy of the good. Because my compiler at work is old (no partial specialization, no boost::function) I haven't been able to integrate into my large-scale code and test it in a real product. I've used it for some projects at home, where it seems to work fine.
Best, John
The latest version of the lib can be found here: http://torjo.com/code/logging.zip
Why is the review version of the library keep changing during review? Where could I see a version submitted for review (6 of November)? Gennadiy
Gennadiy Rozental wrote:
The latest version of the lib can be found here: http://torjo.com/code/logging.zip
Why is the review version of the library keep changing during review?
Where could I see a version submitted for review (6 of November)?
I've uploaded the original version : http://torjo.com/code/logging-initial.zip I've done a few changes during the review, mainly at the docs. 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!
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Gennadiy Rozental | Sent: 14 November 2005 18:15 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | | > The latest version of the lib can be found here: | > http://torjo.com/code/logging.zip | | Why is the review version of the library keep changing during review? Considering how much Gennadiy's Boost Test Library (now Excellent, and still improving, and I trust will continue to improve - especially documentation) has changed since review and acceptance, I feel this 'question' is a bit rich! Paul -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
On Tue, 15 Nov 2005 15:21:54 -0000 "Paul A Bristow" <pbristow@hetp.u-net.com> wrote:
| Why is the review version of the library keep changing during | review?
Considering how much Gennadiy's Boost Test Library (now Excellent, and still improving, and I trust will continue to improve - especially documentation) has changed since review and acceptance, I feel this 'question' is a bit rich!
I think the issue here is that, during review, the submission is supposed to remain unchanged.
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Jody Hagins | Sent: 15 November 2005 15:34 | To: boost@lists.boost.org | Subject: Re: [boost] [Review] The review of the Boost.Logging | librarystartstoday, November 7th | | On Tue, 15 Nov 2005 15:21:54 -0000 | "Paul A Bristow" <pbristow@hetp.u-net.com> wrote: | | > | Why is the review version of the library keep changing during | > | review? | > | > Considering how much Gennadiy's Boost Test Library (now | Excellent, and | > still improving, and I trust will continue to improve - especially | > documentation) has changed since review and acceptance, I feel this | > 'question' is a bit rich! | | I think the issue here is that, during review, the submission is | supposed to remain unchanged. Well I'm not sure it has been in the past? IMO a few minor changes are OK - and desirable even. I don't remember a review when NO changes were required. Nor a product that was never modified - that's why Boost code is good? IMO, as far too often, far too many fundamental issues are being raised far too late in the whole development and review process. Paul -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
| I think the issue here is that, during review, the submission is | supposed to remain unchanged.
Well I'm not sure it has been in the past? IMO a few minor changes are OK - and desirable even. I don't remember a review when NO changes were required.
Changes always required. The point is that you couldn't do them *during* review. It's ok (and nessesary in 100% of reviews) to do them post-review Gennadiy
On Tue, 15 Nov 2005 16:46:02 -0000 "Paul A Bristow" <pbristow@hetp.u-net.com> wrote:
Well I'm not sure it has been in the past? IMO a few minor changes are OK - and desirable even. I don't remember a review when NO changes were required. Nor a product that was never modified - that's why Boost code is good?
Right. However, we have had problems in the past where the original is modified. That's one reason that the original is not supposed to change at all. Mods are almost always needed (compiler workaround or doc updates), but the original submission must stay. We've also had the argument in the past as to what is being reviewed, the newly modified work, or the original. I'm not trying to put a new face on it, just stating that the original submission is always supposed to stay constant (that's what I think the original question was about, anyway).
IMO, as far too often, far too many fundamental issues are being raised far too late in the whole development and review process.
However, we do not have a pre-review stage, so it is difficult to see how these issues would have been raised in the past. I believe our history shows that a submission will be rejected if it is not satisfactory, but when the author has considered the points raised in the review, it can be resubmitted. I guess this is the best we can do, and it seems to be fairly well received.
Hartmut Kaiser <hartmut.kaiser <at> gmail.com> writes:
--------------------------------------------------- Please always state in your review, whether you think the library hould be accepted as a Boost library!
I don't think the library is ready for acceptance yet, but I do think that it embodies many of the elements required to make a widely useful logging library, including some apparently unique/novel ones, and that such a library would make a great addition to Boost. While I think the library could do with some further refinement, the scope of the library needs to be limited sufficiently that it is possible to produce a high-quality implementation. Once the library reaches that quality while offering sufficient flexibility and extensibility it should be accepted. Later submissions of add-ons/enhancements can be included or mini-reviewed as appropriate (new appenders, enhanced/specialised filtering etc). So - thats a "not yet" going on abstain (given that my name is on many of the source files I'm not sure that the review manager should actually count my vote either way).
Additionally please consider giving feedback on the following general topics:
- What is your evaluation of the design?
Under the features there is a pretty good design waiting to get out, however it needs to be revisited in a number of areas: 1) A given message is directed to a particular named log, from where it is sent to 1 or more named appenders. It is easy to connect/disconnect appenders to logs. It is also easy to set the log level. However, I find the log level facility useless, because in general I want to filter the messages to a particular appender, not globally - if one of my appenders is the system error log, I always want errors, all errors, and nothing else, going to it. If I have some sort of console debug output, I might want to turn on debug level output to it alone (in general from only a few logs). I can do this reasonably well if I connect the "system error log" to "*.error" and adjust what goes to the "debug console" by specifying eg. "foo.bar.debug" output to go there, or more likely "foo.bar.*" and turning off "foo.bar.debug" if it is too noisy. If some library author decides that log levels are the way to go, I can't get my app to direct that libraries errors to the error log. Alternatively, if I write a library, and I want to ensure that it is useable in this scenario and in some level-based logging scheme, I would presumably need to log an error with a level of err to a log called "mylib.error" and require that the app using mylib connect appenders to "mylib.*" and set levels on "mylib.*" if the app is using "level based" filtering. While I find level-based filtering limited, I do think the ability to apply some sort of filter other than just a bool enable/disable is useful. The type of filter used should be customisable - a very simple fast filter (bool or just a non-empty appender set) would suit many uses, while some might require a very elaborate check against some sort of filter settings data structure. Either this can be encapsulated as a policy, or (and I think this would be less intrusive and as effective) the enabled_logger etc interface could be tweaked and documented to support something like: #define BOOST_LOGC(log_name, cond) if (logger_keeper keep = logger_and_bool (log_name, (cond) )); else (*keep.stream()) set<pthread_t> threads_to_log; bool thread_enabled() { // needs a mutex return threads_to_log.count(pthread_self()); } BOOST_LOGC(some_log, thread_enabled()) << "Thread " << pthread_self() << " is enabled."; if thread-based filtering is desired by default just replace BOOST_LOG with: #define BOOST_LOG(log_name) if (logger_keeper keep = logger_and_bool(log_name, thread_enabled() )); else (*keep.stream()) BOOST_LOG(some_log) << "Thread " << pthread_self() << " is enabled."; Extending logger_and_bool to logger_and_predicate and the appropriate c'tors for the keeper/enabled_log would allow the predicate to perform checks that require access to the logger itself as well. These tests are (potentially) heavyweight but so long as a simple call to BOOST_LOG simply bypasses them there is no cost to the user who doesn't need them. For example, Gennady would presumably like some form of fully extensible set of attributes to apply filtering rules to based on log name. He could simply maintain whatever map<name_string, attr_type> he needs and write appropriate predicates to filter on this. Of course that could be too slow, in which case a more efficient mapping from log to attributes would be needed - by allowing customisation/extension of the logger_impl - see below. 2) While the library aims to offer a customisation point through replacing the manager type, the manager concept is un(der)documented, and the various interfaces that depend on the manager type are too closely coupled to allow much customisation anyway. Note that this may be ok, if the design of the standard manager is flexible enough without extensive customisation, and any remaining customisation points/concepts are clearly documented. In any case, the use of the customisation needs to be illustrated by example (ie. an alternate manager replacing all the default interfaces). 3) Closely related to the above, I would like to see cleaner separation between the components that make up this library. This is something that is hard to do when the implementation is a feature driven moving target (I've been trying to do it on and off). This separation needs to maintain or increase the existing loose coupling between compilation units that use the library to log, those that set/modify filtering and those that do actual output of logged messages, while reflecting that separation in the implementation. Currrently appenders do this, but the logging and filtering is too internally intertwined. If the library formalised that separation allowing each module to be customised, library code could use logging independent of how the application (or other libraries) used it/controlled it (and without forcing library recompilation). 4) Configuring modifiers using the index to order them is awkward and error- prone. The concept of some sort of indexed insertion into a sequence of modifiers doesn't even seem to be one that there is a real use for - an implied by order of insertion approach would work at least as well. 5) While applying modifiers to logs rather than to appenders is (potentially) more efficient as it avoids doing the modification N times for N appenders, in reality I have yet to encounter a case where the log format wasn't associated with the destination, rather than the source (ie. a log file should have a consistent format). An extreme example is when the log is a structured log such as the windows event log. A simple example is the newline modifier. If an appender is expecting to receive log messages it is up to the appender to delimit the messages in the appropriate way - be that by packing it into a syslog UDP packet or just sticking a newline on the end. I don't think the newline modifier should exist at all, but in other cases it does make sense to include log specific information. I'm not sure that for typical use where N is small that the efficency gains are likely to be significant, but it would also be possible to improve efficiency issue in cases where it is a real concern by using a multiplexing appender (that forwarded formatted output multiple "real" appenders). The modifier concept also blurs the line between insertion of information and formatting - if a log should always contain some particular information at the start of it what is wrong with just writing it to the enabled_logger's stream when it is created. This also leverages all the built-in and extended output and formatting facilities of std streams (including rather more internationalisation support than found in strings). This fits in with the user extensible logger attributes, predicates etc above. I would like to see 2 points at which the content can be modified - when the stream is created, so that informartion that should be a prefix on all logged messages via a particular log, regardless of destination can be inserted there, and another associated with the appender, rather than any particular log, so that any content that must always be inserted in messages to that log can be inserted/formatted there.
- What is your evaluation of the implementation?
There are some areas of the core library, especially robust thread safety, that need attention. The most glaring of these is the m_deleted hack to try to work around the use of a reference in place of a copy of the logger (*not* the logger_impl - copying a logger is cheap - it is only a shared_ptr). There is also an outstanding fixme re. the problem of logger_impls being kept alive by their presence in the logger collection even after they are otherwise out-of-scope. The appenders supplied are, in general, not much more than examples. While this is fine in that user-written appenders are a great customisation point, I think it is really essential that at least a few robust, high performance appenders be offered. I think it is obvious from the above design comments that I think an overall cleanup/refactor reflecting the final design is in order.
- What is your evaluation of the documentation?
The documentation is a nice friendly tutorial. However, there is a severe lack of design documentation supporting rationale, and no real reference section.
- What is your evaluation of the potential usefulness of the library?
A good logging facility is essential in most (I'm tempted to say all) applications of any complexity, and current solutions tend to introduce undesirable coupling or lack features/flexibility. The library is potentially very useful not just as a developer's debugging tool but as a part of any application that needs configurable logging of any sort.
- Did you try to use the library? With what compiler? Did you have any problems?
Yes (an earlier version) gcc 3.3 and 3.4. I didn't have any real problems with it, but I haven't tried to install and use "out of the box" as I was modifying it at the time.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've made a reasonably in-depth study of and changes to versions of this library. I'd say I'm very familiar with it.
- Are you knowledgeable about the problem domain?
Reasonably so - I've written logging components as part of various systems before.
1) A given message is directed to a particular named log, from where it is sent to 1 or more named appenders. It is easy to connect/disconnect appenders to logs. It is also easy to set the log level. However, I find the log level facility useless, because in general I want to filter the messages to a particular appender, not globally - if one of my appenders is the system error log, I always want errors, all errors, and nothing else, going to it. If I have some sort of console debug output, I might want to turn on debug level output to it alone (in general from only a few logs). I can do this reasonably well if I connect the "system error log" to "*.error" and adjust what goes to the "debug console" by specifying eg. "foo.bar.debug" output to go there, or more likely "foo.bar.*" and turning off "foo.bar.debug" if it is too noisy. If some library author decides that log levels are the way to go, I can't get my app to direct that libraries errors to the error log. Alternatively, if I write a library, and I want to ensure that it is useable in this scenario and in some level-based logging scheme, I would presumably need to log an error with a level of err to a log called "mylib.error" and require that the app using mylib connect appenders to "mylib.*" and set levels on "mylib.*" if the app is
I see what you mean. I believe that Gennady's solution of filtering is the way to go. It is very powerful - it took me a while to get it, but now I understand ;)
Extending logger_and_bool to logger_and_predicate and the appropriate c'tors for the keeper/enabled_log would allow the predicate to perform checks that require access to the logger itself as well. These tests are (potentially) heavyweight but so long as a simple call to BOOST_LOG simply bypasses them there is no cost to the user who doesn't need them.
Yup, you're right.
2) While the library aims to offer a customisation point through replacing the manager type, the manager concept is un(der)documented, and the various interfaces that depend on the manager type are too closely coupled to allow much customisation anyway. Note that this may be ok, if the design of the standard manager is flexible enough without extensive customisation, and any remaining customisation points/concepts are clearly documented. In any case, the use of the customisation needs to be illustrated by example (ie. an alternate manager replacing all the default interfaces).
Yes, I definitely need more docs.
3) Closely related to the above, I would like to see cleaner separation between the components that make up this library. This is something that is hard to do when the implementation is a feature driven moving target (I've been trying to do it on and off). This separation needs to maintain or increase the existing loose coupling between compilation units that use the library to log, those that set/modify filtering and those that do actual output of logged messages, while reflecting that separation in the implementation. Currrently appenders do this, but the logging and filtering is too internally intertwined. If the library formalised that separation allowing each module to be customised, library code could use logging independent of how the application (or other libraries) used it/controlled it (and without forcing library recompilation).
Yes, as I will redesign it, this will definitely happen.
5) While applying modifiers to logs rather than to appenders is (potentially) more efficient as it avoids doing the modification N times for N appenders, in reality I have yet to encounter a case where the log format wasn't associated with the destination, rather than the source (ie. a log file should have a consistent format). An extreme example is when the log is a structured log such as the windows event log. A simple example is the newline modifier. If an appender is expecting to receive log messages it is up to the appender to delimit the messages in the appropriate way - be that by packing it into a syslog UDP packet or just sticking a newline on the end. I don't think the newline modifier should exist at all, but in other cases it does make sense to include log specific information. I'm not sure that for typical use where N is small that the efficency gains are likely to be significant, but it would also be possible to improve efficiency issue in cases where it is a real concern by using a multiplexing appender (that forwarded formatted output multiple "real" appenders).
I still like the modifier concept, but I do see your point. In the future, I'd like to provide modifiers, which I think are useful impelmented alone (like: newline, prepend_time, etc.), BUT, have the appenders call them, if needed.
The modifier concept also blurs the line between insertion of information and formatting - if a log should always contain some particular information at the start of it what is wrong with just writing it to the enabled_logger's stream when it is created. This also leverages all the built-in and extended output and formatting facilities of std streams (including rather more internationalisation support than found in strings). This fits in with the user extensible logger attributes, predicates etc above. I would like to see 2 points at which the content can be modified - when the stream is created, so that informartion that should be a prefix on all logged messages via a particular log, regardless of destination can be inserted there, and another associated with the appender, rather than any particular log, so that any content that must always be inserted in messages to that log can be inserted/formatted there.
Yes, when I'll redesign the lib, I'll make it possible to format the string at any time. Thanks for the 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!
participants (1)
-
Andrew Schweitzer -
Caleb Epstein -
Darryl Green -
Eelis van der Weegen -
Felipe Magno de Almeida -
Gennadiy Rozental -
Hartmut Kaiser -
Jody Hagins -
John Torjo -
João Abecasis -
larsbj＠gullik.net -
Michael van der Westhuizen -
Paul A Bristow -
Pavel Vozenilek