On Sunday 10 March 2013 19:56:19 Vicente J. Botet Escriba wrote:

The review result requested to take in account some critical issues. I'm sure that we could found some of the answers to these question on the documentation, but it would be easier for the mini-review if you give some explicit answers to these critical issues. Telling if you have taken in account some of the non-critical points could help also.

Sure.

Critical issues ===============

These issues must be addressed, and be passed through informal mini- review, before the library can be added to SVN.

- It appears that the 'trivial' logging is not sufficiently good to just take and use. The library should provide an out-of-the-box mechanism that:

- supports channel/severity attributes, without creating new logger for each severity.

The point about different severity levels requiring different loggers is not accurate. This was never the case. But it was so for channels. In v2, it is now possible to change channel for an existing logger without creating a new one. A new macro BOOST_LOG_CHANNEL is introduced which allows to set channel name for a specific log record. http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed/sources.html... However, I still don't recommend doing this in performance critical places.

It also should be possible to filter out specific channel by essentially poking at some "map", and without manually composing a new filter that checks for a set of allowed channels sequentally. I believe that at least Christian, Roland, Robert and myself requested this.

This is implemented with the channel_severity filter. http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed/expressions....

- has no observable effect on compile time. One second (or more) that trivial.hpp imposes is not acceptable.

I've done my best to reduce compile times, although I didn't purposefully measure the gain. Compiling libs/log/example/trivial takes ~1.75 second on my setup. Commenting out the expressions part of the example reduces the time to ~0.42 s. I'm open to suggestions for further improvements.

- outputs to console by default, but permits changing that

Done. The default sink outputs to console. When other sinks are added they override the default sink.

- outputs only component/severity/message by default, but probably permits changing that.

The default sink format is the same as before: timestamp [thread id] [severity] message I believe, all these components are essential for common logging purposes. You can always set up your own format when you register your sinks.

- By default, the library should try hard to continue working no matter what errors happen during processign a log record -- including things like invalid attribute names or types, or other exceptions.

Done, filters and formatters don't throw by default.

- The library should not reinvent wheels. In particular, custom implementations of TSS and RW mutex seems like a very bad idea. Use of a custom lambda implementation was pointed by many as not great -- we already have a couple, so let's not add more to the mix.

The lambda expressions (filters and formatters) are now based on Boost.Phoenix. Custom implementations of TSS and RW mutex are preserved as I'm not satisfied with the existing implementations in Boost.Thread. We discussed this with Vladimir after the review report was published and agreed that this was not a hard requirement.

- Library interface is too geared towards lambda. In particular, implementing formatters/filters as free-standing functions appears to require considerably more code. In particular, it should be possible to access an attribute using a single function call. Also, logging::extract taking a lambda for callback complicates matters, it's desirable to have a more direct variant.

The previous extraction API has been reworked and divided into new extraction and visitation APIs. In general, the former one allows to get a reference to the stored value, and the latter one is for applying a visitor to it. http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed/attributes.h... Also, with addition of attribute keywords it is now possible to extract attribute values with operator[] applied to records and attribute value sets. http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed.html#log.det... As for other issues in the report: Important issues ================ These issues are important enough to be mentioned here, but can be addressed as convenient. - The reported run-time performance can be improved. In particular the performance with date attributes seems scary and 2x slowdown relative to some other library in the 'most records are filtered out' case is also concerning. I have implemented a custom version of the date/time formatter that does not use facets provided by Boost.DateTime. I did not measure the performance in numbers but it should be much faster. - I don't think it was satisfactory explained why 'attr' exists in two namespaces, as opposed to being a single type that is used for both formatting and filtering. This cross-references the custom lambda expressions that were in v1. Now there is a common boost::log::expressions namespace and a common 'attr' placeholder in it. Other notes =========== These issues seem important and should be considered, but might not even be fixable within the current design. - Several reviewers said the library is "too flexible" and this makes it harder to understand. Probably nothing can be done about that at this point, but interesting data point notwithstanding. Well, flexibility comes from multitude of requirements to the library. I don't think reducing flexibility would serve the library for the better, but I admit that some things are more complicated than I'd like. - It was suggested that the library include an API that another Boost log may use in a way that permits plugging another logging library should end user of a Boost so desire. This seems a good idea, but I think it's up to Andrey to implement such API or not. Any logging library can use the regular logging interfaces Boost.Log provides. There's no need for a special interface. - Alternative design approaches were proposed, in particular (i) permitting user log records and templating/basing other components on a user- defined record type and (ii) using compile-time indexing to access attributes. Also, it was proposed that named parameters not be necessary to use the library, and a more conventional interface be available. It does not seem that implementing those suggestions is fully possible without writing a different library from scratch, so it's up to Andrey to consider those proposals. This would take to write another library for other purposes. - It was suggested to use newer Spirit to address poor compile times for the library itself. I've ported the code to Boost.Spirit v2 although I don't think this improved library compile times.

Am 11.03.2013 04:17, schrieb Andrey Semashev:

On Monday 11 March 2013 02:24:44 Darren Garvey wrote:

...

Hi Andrey,

I'm glad you've added the trivial logger. As big as that warning is in the documentation, it would be more reassuring to see that this logger supports lazy evaluation of the logging expression if the example demonstrated it. eg.

// Note: some_very_expensive_operation() won't be called unless debug logging is enabled. BOOST_LOG_TRIVIAL(debug) << "A debug severity message" << some_very_expensive_operation();

Regardless of the logger you use, that's the case; the streaming expression won't be evaluated if the filter does not pass the record. It's mentioned in the docs in several places.

http://boost-log.sourceforge.net/libs/log/doc/html/log/rationale/why_not_laz... I came away from this thinking that the library doesn't support conditional evaluation at all. Maybe you can clarify that you chose to implement conditional evaluation not by lazy streaming but in another way. What I find a little strange interface-wise is logging::core::get()->set_filter(...) I haven't followed the review process so maybe there is good reason for this. How many cores are there? if it's a singleton, as the documentation of get() says, why does it return a pointer? can it be a nullptr? and isn't whether the core is a singleton object or not an implementation detail anyway? then it could be a static function logging::core::set_filter(...)? The documentation is clear and to concise for someone only interested in trivial logging. Stefan

Am 11.03.2013 10:13, schrieb Andrey Semashev:

...

What I find a little strange interface-wise is logging::core::get()->set_filter(...)

The core is a singleton, yes. The pointer is returned to allow to lock the core from destruction on program termination. The core won't be destroyed while there still are loggers.

then it's an implemention detail, I'd suggest making all the functions of logging::core static, or make logging::core a namespace. if you wanna add the feature you've mentioned later on, you could for example ask the user to construct and hold a core::log_during_termination object, which - again as an implementation detail - holds a shared_ptr to the internal core.

Not that the library works ok on program termination, but the existing problems are not related to the premature core destruction, and this pointer technique lays the foundation for further improvements in this area.

I had the idea of making the core default constructible so that it would contain the pointer to the core singleton internally. But that seemed a counterintuitive design to me as noone expects a default constructible object to be actually a singleton.

This is the Monostate pattern, isn't it? See e.g. http://c2.com/cgi/wiki?MonostatePattern Best regards, Gareth ************************************************************************ The information contained in this message or any of its attachments may be confidential and is intended for the exclusive use of the addressee(s). Any disclosure, reproduction, distribution or other dissemination or use of this communication is strictly prohibited without the express permission of the sender. The views expressed in this email are those of the individual and not necessarily those of Sony or Sony affiliated companies. Sony email is for business use only. This email and any response may be monitored by Sony to be in compliance with Sony's global policies and standards

Am 11.03.2013 12:51, schrieb Andrey Semashev:

...

then it's an implemention detail, I'd suggest making all the functions of logging::core static, or make logging::core a namespace. if you wanna add the feature you've mentioned later on, you could for example ask the user to construct and hold a core::log_during_termination object, which - again as an implementation detail - holds a shared_ptr to the internal core.

This won't work because every static function would have to obtain the pointer to the core which may be invalidated because its global destructor has been invoked already.

I'm not suggesting any changes to the internals, only to the interface. Maybe the following code makes it a little clearer: class core{ public: static void set_filter(){ get()->set_filter_impl(); } private: static shared_ptr<core> get(); void set_filter_impl(); friend class log_during_termination; }; class log_during_termination{ public: log_during_termination() : hold(core::get()){} private: shared_ptr<core> hold; }; user code: ----------- A(){ public: ~A(){ //logging core is not destructed yet } private: log::log_during_termination logterm; } static A a; Best regards

Am 11.03.2013 13:27, schrieb Stefan Strasser:

Am 11.03.2013 12:51, schrieb Andrey Semashev:

...

...

then it's an implemention detail, I'd suggest making all the functions of logging::core static, or make logging::core a namespace. if you wanna add the feature you've mentioned later on, you could for example ask the user to construct and hold a core::log_during_termination object, which - again as an implementation detail - holds a shared_ptr to the internal core.

This won't work because every static function would have to obtain the pointer to the core which may be invalidated because its global destructor has been invoked already.

I'm not suggesting any changes to the internals, only to the interface. Maybe the following code makes it a little clearer:

was your point that the destructor A::~A in my code can't use the static functions? that's correct, but it can't use the current "static core::get()" either. log_during_termination can provide an interface for this corner case.

Am 11.03.2013 13:48, schrieb Andrey Semashev:

On Mon, Mar 11, 2013 at 4:35 PM, Stefan Strasser <strasser@uni-bremen.de> wrote:

...

Am 11.03.2013 13:27, schrieb Stefan Strasser:

...

Am 11.03.2013 12:51, schrieb Andrey Semashev:

...

...

then it's an implemention detail, I'd suggest making all the functions of logging::core static, or make logging::core a namespace. if you wanna add the feature you've mentioned later on, you could for example ask the user to construct and hold a core::log_during_termination object, which - again as an implementation detail - holds a shared_ptr to the internal core.

This won't work because every static function would have to obtain the pointer to the core which may be invalidated because its global destructor has been invoked already.

I'm not suggesting any changes to the internals, only to the interface. Maybe the following code makes it a little clearer:

was your point that the destructor A::~A in my code can't use the static functions? that's correct, but it can't use the current "static core::get()" either.

Exactly. Loggers hold the pointer to the core and use it to emit log records. If you cache a logger as a member in your A, you will not have to call core::get() in ~A. But you will have to call some core methods (through the logger) to write logs in ~A.

okay, if the loggers don't use get() but hold a shared_ptr (and aren't destructed yet themselves) the problem doesn't exist anyway. then the non-static functions have to be public for the loggers to use. I wasn't trying to get into a discussion about how logging during termination is best implemented, all I'm saying is that such a corner case like logging during termination should not force a regular user to use a rather strange interface. none of the internals, however they may be, have to change for that.

As said before I'm upgrading to V2 and adapting my code accordingly. I have a problem I can't figure what exactly is the source (I'm using VS2012 Update1): namespace blog = boost::log; namespace expr = blog::expressions; const auto FORMAT_MODULE_NAME = expr::attr< std::string >( logattr::MODULE_NAME ); const auto FORMAT_TIMESTAMP = expr::format_date_time<boost::posix_time::ptime>( logattr::TIMESTAMP, "%Y-%m-%d %H:%M:%S" ); const auto FORMAT_MESSAGE = expr::message; const auto a = expr::stream << FORMAT_TIMESTAMP; // compiles const auto c = expr::stream << FORMAT_MODULE_NAME; // compiles const auto d = expr::stream << FORMAT_MESSAGE; // error: no operator "<<" matches these operand const auto e = expr::stream << "test"; // error: no operator "<<" matches these operand const auto f = expr::stream << std::string("test"); // error: no operator "<<" matches these operand Any idea what I'm missing? It was working with previous versions. Joel Lamotte

On Mon, Mar 11, 2013 at 4:19 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:

Could you send me a complete compilable code sample?

Yes, I just tried with the following code which is not as minimal as it could be (it the headers includes a lot other headers) but it generate the same error in an empty project (generated with CMake) Note that I upgraded to Boost Log V2 in the same time than Boost from v1.52 to v1.53. The problem seem related to Phoenix as stream is just a typedef if I understand the code correctly. I added the compilation error log in case the problem comes from something I've setup: http://pastebin.com/zHRwaBsQ ------ #include <boost/log/expressions/formatters.hpp> #include <boost/log/expressions/formatters/stream.hpp> #include <boost/log/sinks.hpp> namespace logattr { const auto MODULE_NAME = "ModuleName"; const auto TIMESTAMP = "TimeStamp"; } int main() { namespace blog = boost::log; namespace expr = blog::expressions; const auto FORMAT_MODULE_NAME = expr::attr< std::string >( logattr::MODULE_NAME ); const auto FORMAT_TIMESTAMP = expr::format_date_time<boost::posix_time::ptime>( logattr::TIMESTAMP, "%Y-%m-%d %H:%M:%S" ); const auto FORMAT_MESSAGE = expr::message; const auto a = expr::stream << FORMAT_TIMESTAMP; // compiles const auto c = expr::stream << FORMAT_MODULE_NAME; // compiles const auto d = expr::stream << FORMAT_MESSAGE; // error: no operator "<<" matches these operand const auto e = expr::stream << "test"; // error: no operator "<<" matches these operand const auto f = expr::stream << std::string("test"); // error: no operator "<<" matches these operand return 0; }

On Mon, Mar 11, 2013 at 5:16 PM, Andrey Semashev <andrey.semashev@gmail.com>wrote:

You also have to #include <boost/phoenix/operators.hpp> as Boost.Phoenix does not include its operator overloads by default. Or simply #include <boost/log/expressions.hpp> which takes care of that.

Thanks, it fixes the problem in production code too. Joel Lamotte

On Monday 11 March 2013 17:43:00 Klaim - Joël Lamotte wrote:

Actually I have to be more precise:

#include <boost/phoenix/operators.hpp>

Is not found,

Sorry, it should be <boost/phoenix/operator.hpp>. My bad.