Re: [boost] Review of Logging library begins today February 4th

From: John Torjo (john.groups_at_[hidden])
Date: 2008-02-05 07:48:42

Steven Watanabe wrote:
> AMDG
>
> Gennadiy Rozental wrote:
>   
>> The formal review of Logging library, proposed by John Torjo, begins today and 
>> will run till Feb 13:
>>   
>>     
>
> Here's part 1 of my review from reading the docs only.
> More to follow after I look through the implementation.
>
>
> in main_intro.html there is a line that says
>    Click to see the code
> which sounds like it ought to be a link but isn't.
>
>   
Yup, sorry - fixed. Will be online shortly.
> scenarios_code.html#scenarios_code_mom:
> I don't think that write is the correct name:
>    g_l()->writer().write("%time%($hh:$mm.$ss.$mili) [%idx%] |\n", "cout 
> file(out.txt) debug");
> This doesn't write anything.  It seems like this should be
> called something more like set_format_and_destination()
>
>   
Yup you're right - will fix it after review.
> I don't like the idea of two stage initialzation implied by
>    g_l()->mark_as_initialized();
> Is there any way that the initialization can be done where
> the log is defined?
>
>   
See http://torjo.com/log2/doc/html/caching.html for the rationale.
However, for a named_logger, we might be able to avoid caching - that 
is, initialize it in one step.

Note that in the future I want to make caching a policy - I just didn't 
have time to do it before the review.
> Concepts as Namespaces:
> I strongly dislike this way of expressing it.  What you mean is
> that you put implementations of that concept in a specific
> namespace.  The concept, per se, has no relation to the namespace.
> This description is very misleading, IMO.
>
>   
Actually I think this is way too cool. First of all, it's very easy to 
find everything that is related to a concept:
- in the documentation - just click on that namespace
- when using code completion

In fact once you get used to the library, you'll see that gets easier 
and easier to read the code/docs.
I don't know if any other lib has done this before - but please give it 
a shot first ;)
   
> What does the suffix ts stand for?  Oh. I see.  Thread Safety.
> Could this be spelled out completely?  It's not going to appear
> all over the code so there is no reason to make the name as short
> as possible.
>
>   
Ok, I can update this - after the review.
> boost::logging::formatter::idx:
>    Please! don't abbreviate this. "index" is only *two* characters longer.
>
>   
Will do - after the review
> Workflow.html:
> You'll use this when you want a optimize string class. Or, when using tags
>   s/optimzed/optimized/
>   s/a/an/
>
> Is there a way to have destination specific formatters?
>
>   
Yes. When using a custom route (msg_route::with_route). If you make 
optimize::cache_string_several_str<> your msg formatter.
See http://torjo.com/log2/doc/html/no__levels__with__route_8cpp-example.html

> Throughout the docs some #include's directives are links to the
> corresponding headers and other are not.
>
>   
My mistake - please see 
http://torjo.com/log2/doc/html/headers_to_include.html
That's all you need to know.
> I find the use of preprocessor directives vs. templates for customization
> not very intuitive.  For example, the options controlling caching are 
> macros.
> I'm not convinced that this should be a global setting.  The fast and
>   
About cache - see above.
> slow compile time options make sense I think because they are
> transparent to the user.  (BTW, would I need to #ifdef out the
> #include <boost/logging/format.hpp> to have the compile times improve?)
>
>   
What do you mean?

> AT the bottom of macros.html:
>    #define BOOST_LOG_TSS_USE_CUSTOM = my_thread_specific_ptr
> The "=" is wrong I think.
>   
At this time it's ok. However I guess you're right - after the review I 
can update the code not to require that '='.

> If BOOST_LOG_NO_TSS is defined is it an error to use the 
> thread-specific-storage filters?
>
>   
Yes.
> filter::debug_enabled/release_enabled: How is debug/release determined.  
> NDEBUG?
>
>   
Yes
> in namespaceboost_1_1logging_1_1manipulator.html there is
> a reference to destination::shared_memory - writes into shared
> memory (using boost::shmem::named_shared_object)  is this obsolete?
>
>   
I need to rewrite it to use interprocess.
> namespaceboost_1_1logging_1_1manipulator.html#manipulator_share_data:
> It doesn't look like the example here will compile.
>   
You're right - instead of "write_to_file"  I should have written "my_file"
> Second, is there any particular reason not to use shared_ptr directly?
>
>   
Yes, see this : 
http://torjo.com/log2/doc/html/namespaceboost_1_1logging_1_1manipulator.html#manipulator_manipulate
> structboost_1_1logging_1_1manipulator_1_1is__generic.html:
> I'm don't understand the reference to the template operator=.
>
>   
Not sure I understand what you mean.
> namespaceboost_1_1logging_1_1formatter_1_1convert.html
>   "explain that you can extend the following - since they're 
> namespaces!!! so
>   that you can "inject" your own write function in the 
> convert_format::prepend/or
>   whatever namespace, and then it'll be automatically used!"
> I'm almost certain that this is wrong.  In order to be found by a template
>   
It's not - see http://torjo.blogspot.com/2007/10/template-construct.html
>
> Concepts:
> For the following concepts I either couldn't figure them out
> or had to hunt all over the documentation.  This information
> should be specified in the Concepts section
>
>   
I ssume you've read the Workflow, right?
> Filter:  There is some way to get a boolean out of a filter. All the 
> details are
>          specified by the logging macro.
>   
The macro uses the filter.
The filter can have a function that returns a boolean. It can have no 
parameters (for instance, if it's a simple filter), or for instance one 
parameter if it's a filter based on levels.
> Level:   I can't figure out from the documentation what I would need to do
>          to model this concept.
>   
http://torjo.com/log2/doc/html/namespaceboost_1_1logging_1_1level.html

(also, see scenario below)
> Writer:  Seems to be a specialization of a Unary Function Object?
>
>   
Yes
> Scenario: This doesn't seem like a concept.  I'm a little confused now.
>           In addition I don't understand why ts and usage are disjoint.
>           There seems to be significant overlap between the two.  Is there
>           any way that they could be merged?
>   
"scenario" is a way for you to decide the right logger, based on your needs:
- "usage" - you can find the right logger, based on usage, like:

using namespace boost::logging::scenario::usage;
typedef use<
        //  the filter is always accurate (but slow)
        filter_::change::always_accurate,
        //  filter does not use levels
        filter_::level::no_levels,
        // the logger is initialized once, when only one thread is running
        logger_::change::set_once_when_one_thread,
        // the logger favors speed (on a dedicated thread)
        logger_::favor::speed> finder;

- "ts" - you can find your logger, based on your thread-safety needs, like:
using namespace boost::logging::scenario::ts;
typedef use< filter_::use_tss, level_::no_levels, logger_::use_tss> finder;

>
>
> Gather: What exactly are the requirements?  The docs say "a function 
> that will
>         gather the data - called .out()" What are the requirements on the
>         return type?  Further in scenarios_code.html#scenarios_code_mon 
> there
>         is the definition void *out(const char* msg) { m_msg = msg; 
> return this; }
>         Is out() called implicitly or not?  Why is "this" returned as a 
> void*?
>         Confused...
>   
The requirements are to return a void* pointer.
This is needed when http://torjo.com/log2/doc/html/caching.html and you 
want to cache the filter as well.

If I remove the usage of 
http://torjo.com/log2/doc/html/caching.html#caching_BOOST_LOG_BEFORE_INIT_CACHE_FILTER
then you can return anything you like.
For more details, please take a look at cache_before_init_macros.hpp, 
line 50
> Tag: The only requirement on a tag is that it must be CopyConstructable 
> (Is it
>      Assignable, too?).  tag::holder is a effectively a fusion::set of tags.
>   
Not really - theres'a  bit of logic in that class.
>      It's not clear from either the tag or holder documentation how to 
> extract
>      tags in a formatter.
>   
Not sure what you mean.
>
> One final comment about the Concepts:
>
> Manipulators: I think this framework is overly complex.  It would be 
>   
Why?
> better to
>               make manipulators Equality Comparable Unary Function Objects.
>               In the case of destinations the return type is ignored, for
>               for formatters, the result type should be the same as the 
> argument
>               type.  For both the argument type is determined from the 
>   
"for formatters the result should be the same as the argument type"  - why?
> logger.
>               You can use type erasure to allow them to be treated 
> polymorphically
>               at runtime.
>   
Isn't this what I'm doing?

Best,
John

-- 
http://John.Torjo.com -- C++ expert
http://blog.torjo.com
... call me only if you want things done right