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

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2008-02-05 01:35:25

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.

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()

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?

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.

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.

boost::logging::formatter::idx:
   Please! don't abbreviate this. "index" is only *two* characters longer.

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?

Throughout the docs some #include's directives are links to the
corresponding headers and other are not.

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
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?)

AT the bottom of macros.html:
   #define BOOST_LOG_TSS_USE_CUSTOM = my_thread_specific_ptr
The "=" is wrong I think.

If BOOST_LOG_NO_TSS is defined is it an error to use the 
thread-specific-storage filters?

filter::debug_enabled/release_enabled: How is debug/release determined.  
NDEBUG?

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?

namespaceboost_1_1logging_1_1manipulator.html#manipulator_share_data:
It doesn't look like the example here will compile.
Second, is there any particular reason not to use shared_ptr directly?

structboost_1_1logging_1_1manipulator_1_1is__generic.html:
I'm don't understand the reference to the template operator=.

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
the overloads need to be declared by the point where the template is 
defined,
although if I remember correctly not all compilers implement this correctly.
If you're going to rely on overloading for customization do it
via ADL.

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

Filter:  There is some way to get a boolean out of a filter. All the 
details are
         specified by the logging macro.

Level:   I can't figure out from the documentation what I would need to do
         to model this concept.

Writer:  Seems to be a specialization of a Unary Function Object?

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?

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...

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.
     It's not clear from either the tag or holder documentation how to 
extract
     tags in a formatter.

One final comment about the Concepts:

Manipulators: I think this framework is overly complex.  It would be 
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 
logger.
              You can use type erasure to allow them to be treated 
polymorphically
              at runtime.

In Christ,
Steven Watanabe