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