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.
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.
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.
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. The attribute value interface offers the "get" convenience method that returns optional. But, yes, you'd have to find the attribute value first in the view.
Is it possible to declare and define a global logger separately?
No. But it seems to be a reasonable improvement. Thanks.
I think LineID may be a bad name, since when I saw it, I assumed it meant the line in the source that generated the log message, perhaps RecordID would be better?
That should be ok.
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.
I don't like the use of a time in ordering_asynchronous_sink to specify how long to wait for a new record. It just seems too fragile. Suppose that the machine is running under a heavy load or I suspend the process, or put the machine to sleep? I don't like having the correctness depend on how long things take. If nothing else, it needs to be stated that ordering_asynchronous_sink is only a "best effort" mechanism and makes no guarantees.
The time-based window has much more sense than a record count-based one, for the reason you have pointed out. If the application is actively emitting log records, the window gets bigger in order to accommodate more records that may go out of order. If records are emitted once in a while, the window is nearly empty all the time. The time measurement also gives the idea of the sink minimum latency.
fmt::xml_dec. I would prefer a more intuitive name like xml_encode or xml_escape. (Since dec is "obviously" an abbreviation of decrement)
"Dec" stands for "decorator". But xml_escape is also fine.
In http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed/sink_backend...,
the documentation says that custom_level_mapping works for integral types, but the example uses std::string.
Yeah, that section needs updating. It's custom_severity_mapping, actually, and it really does support non-integral types.
Some of the code examples overflow the window.
Which ones? Also, what resolution should I target? I'm using 1280x1024, and everything looks ok.
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?
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. * The resulting code looked heavier than my current solution. * I didn't see much advantage in this port.
For instance satisfies, would be better as bind(f, attr<...>(...));
I think, "satisfies" is more telling than the binding you suggest.
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. :)
You claim in http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed/utilities.ht...
that std::type_info is required to be unique for each type. I could not find such a requirement in the standard. Please provide the reference.
I guess, I was inaccurate. It's non-copyable and non-assignable.
Also, std::type_info has no member raw_name in the standard. If it exists, it's an extension.
Right.
Why is type visitor::visit not spelled operator()?
No reason in particular. This can be changed.
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.
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?
In the todo section I noticed a reference to #3278. This was fixed a while ago.
Yes, but not in Boost releases the library is currently compatible with. If Boost.Log gets accepted, I will move to Boost.Xpressive, the code is there, but commented.
In http://boost-log.sourceforge.net/libs/log/doc/html/log/extension.html under the minimalistic sink backend, the description of consume doesn't seem to match the code: "The record, as it was stated before, contains a set of attribute values (goes as the first argument of the function) and the message string (goes as the second one)." vs. void stat_collector::consume(record_type const& rec);
Right, it needs updating.
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.