Re: [boost] [log] review part 2

Subject: Re: [boost] [log] review part 2
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2010-03-15 16:21:11

AMDG

Andrey Semashev wrote:
> On 03/15/2010 08:22 AM, Steven Watanabe wrote:
>>
>> I thought that the Boost.Parameter convention is
>> to give keywords names starting with an _.
>
> I think, some time ago there were two recommendations in the 
> Boost.Parameter docs - the underscore and the namespace. Personally, I 
> don't like leading underscores as it always makes me think of some 
> implementation details.

The Boost.Parameter docs seem to recommend using both together.
http://www.boost.org/libs/parameter/doc/html/index.html#keyword-naming

>> I think that it would be a little nicer
>> to work with attributes with an interface
>> like this:
>>
>> BOOST_LOG_ATTRIBUTE(logging::trivial::severity_level, severity);
>> BOOST_LOG_ATTRIBUTE(unsigned int, line_id);
>>
>> fmt::stream
>> << fmt::line_id()
>> << ": <" << fmt::severity()
>> << "> " << fmt::message()
>>
>> That way, it's easier to guarantee static type safety and
>> is also a little less verbose when accessing the attributes.
>
> I thought if that. It was not done for two reasons:
>
> 1. The macro would have to have a formatter in its arguments. Given 
> that the attribute definition should be exposed rather widely, this 
> additional dependency would be unwelcome.
> 2. Guys that hate macros would start complaining.

You don't seem particularly shy about using macros in
other places.  This is a fairly standard use of macros
in Boost.

>> Since you allow use of Boost.Format, I don't think that
>> the extra format argument of attr is necessary.
>
> Boost.Format is currently invoked on strings, composed by attr 
> formatter. Also, allowing to specify format even for streaming 
> formatters looks like a nice feature.

My comment here was actually tied to the one before.  I'm
willing to drop support for automatic invocation of Boost.Format,
since it can always be handled explicitly.

>> I would like logging::extract< unsigned int > to return
>> a boost::optional or something like that instead of having
>> an output parameter.
>
> This would require the attribute value to be copyable. Also, "extract" 
> may accept a MPL sequence of types, in which case it would have to 
> return something like optional< variant< types > >. This looks like an 
> overkill.

Oh, right.

>> I don't like the inconsistency of
>> logging::make_attr_ordering< unsigned int >("Line #").
>> I though the name was LineID?
>
> The names may differ, the point is the same. I didn't try to force any 
> names with my code samples. Users are free to decide which names fit 
> them best.

Given that LineID is one of the common attributes that
you define, I don't see the point.

>> Some of the code examples overflow the window.
>
> Which ones? Also, what resolution should I target? I'm using 
> 1280x1024, and everything looks ok.

You should aim for about 80 columns (a little more is okay), so that
it looks okay in a pdf build.  I've put a compiled pdf of the documentation
at http://www.cs.hmc.edu/~swatanabe/log.pdf.  There's some line wrapping
on pages 83, and 85, for instance.

>> Why are there separate classes for handling the syslog and
>> windows event log mappings? It seems like they're basically
>> doing the same thing.
>
> They are implemented with the common base class. Different top-level 
> classes are defined in order to help with type safety. The 
> custom_severity_mapping returns syslog::level_t, and mappings for 
> event log return their corresponding types.
>
>> Using scoped attributes to add attributes to a specific
>> log record seems like a hack.
>
> Why?

Because it doesn't match the intuitive idea of what
you're trying to do.

>> I don't really like to see the lambda capabilities implemented
>> from scratch. At the very least, I'd like to see plans for
>> porting it to Phoenix v3 once it's ready. A full lambda library
>> is far more feature complete than the minimal lambda that
>> you provide.
>
> I don't know about Phoenix 3, but when it's released, I'll take a look.
> I considered and even tried to port on top of Proto, but dropped in 
> the end for several reasons:
>
> * I couldn't figure out how to make Proto expressions to be function 
> objects.

http://www.boost.org/doc/html/proto/users_guide.html#boost_proto.users_guide.front_end.adding_members_by_extending_expressions

> * The resulting code looked heavier than my current solution.
> * I didn't see much advantage in this port.

We have no need for yet another lambda library in Boost.

> > For instance satisfies, would be better
> > as bind(f, attr<...>(...));
>
> I think, "satisfies" is more telling than the binding you suggest.

Perhaps it is, but satisfies is another name to learn, while bind
should already be well-known, and is much more flexible.

>> For string_literal, I seem to recall that Boost.Test already
>> has a basic_cstring. Maybe this should be factored out
>> instead of being duplicated.
>
> There are several tools in the utility folder that could be extracted 
> from the library. I once asked if there is any interest in it on this 
> list and got no response.
>
>> I wish that the names of functions and classes linked to the
>> doxygen reference.
>
> Sure, if I figure out how to do it. :)

I usually use something like [def __basic_core__ [classref 
boost::log::basic_core basic_core]]
Also, in various places where you mention the examples directory, it 
would be nice if you
could link to the source file for the specific example that you're 
referring to.

>> The stuff for extracting the values of attributes is so
>> complex. Why can't you just have a single interface like
>> visit<boost::mpl::vector<int, char, double> >(attr, f);
>> rather than providing all these complicated layers.
>> I guess that doesn't work when the set of possible types
>> isn't known at compile time, but I still think it should
>> be possible to cut down the number of redundant public
>> interfaces.
>
> What interfaces do you have in mind?
> Type dispatchers are the lowest level tools. Attribute values 
> dispatching builds on top of it. Your 'visit' seems something in 
> between the type dispatchers and 'extract'. It so happened, I didn't 
> need such middle-placed tool and thus didn't make it.

You have extract, attribute_value_extractor, static_type_dispatcher,
and dynamic_type_dispatcher.  AFAICT, attribute_value_extractor and
static_type_dispatcher serve exactly the same purpose.  I don't
see a good reason to have both as public interfaces.

>> I'm not convinced that make_exception_handler belongs in
>> this library.
>
> Huh? Where does it belong then? Or do you mean it could be extracted 
> into a separate library?

I don't think that it belongs anywhere.  This is a general
purpose utility that has no particular relation to logging
and is not used by the library except in the tests.  I
don't think that using it is any better than

struct my_exception_handler {
    void operator()() const {
        try {
            throw;
        } catch(std::exception& e) {
            // some code
        }
    }
};

>> What happens if...
>>
>> A filter or formatter tries to log something.
>
> Bad things may happen. The logging library should not log. Or at 
> least, log so that no recursion occurs.

I figured that it was a bad idea.  I was just curious.

In Christ,
Steven Watanabe