Boost logo

Boost :

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

Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk