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