John Torjo wrote:
* What is your evaluation of the design?
In short, questionable. More elaborate notes follow:
* To my mind, the named destination design approach is not intuitive.
Right, because that's how you implemented you lib. That is so biased.
Well, believe me, I tried to be as objective as I could. Look at it from another side, I wouldn't have started my lib if I was happy with another (yours?) solution. I'm missing some features and that's what I expressed in my review. And since it is your library review I propose to concentrate on your solution, not mine. Note to the review manager: You are free to decide whether to take my vote into consideration or not. Any decision would be fine with me.
First, if that were true, we should ban scripts, 'cause you might do typos.
Every source of errors is evil. Especially the ones that show in run time. If we can get rid of them, why not?
Second, how many times do you initialize a logger? Oh yes, just once...
That's a tricky question. I'd like to have one such place. But first, as I've noted below I seem to have such code in each module. Second, even this single piece of code will be modified some day, and the one who modifies it may introduce an error.
Third, this allows you to initialize the logging from a configuration - which your lib can't quite do.
I was talking about this kind of code: g_l()->writer().add_destination( destination::named("cout out debug") .add( "cout", destination::cout()) .add( "debug", destination::dbg_window() ) .add( "out", destination::file("out.txt")) ); Ok, I agree that it is _possible_ to initialize it after reading the file, but not convenient (at least for me). I don't see why would I need to duplicate these "cout out debug". Actually, I don't see why would I need these names in the first place.
time. It also complicates making the list of destinations configurable (e.g. loading logging settings of the user's app from a file).
* The approach to adding formatters is not very intuitive.
That's why I have the named syntax - which of course, you dislike.
Named formatter syntax, I missed that... You have a point here, although I would really like a lambda syntax support along with it.
* Lifetime of the objects provided by users, such as streams, should be controlled by the library.
That's easy - I'll have a destination::stream_ptr - it's on my TODO list.
Why would you need yet another smart pointer?
However, what if you just have a raw pointer to an existing stream, and you just can't get a shared_ptr?
How would you get such raw pointer? Anyway, in case if someone else is controlling the stream life time you can always provide an empty deleter to shared_ptr and keep your headache.
* Filtering looks way too minimalistic to me.
What? I really don't get this. A filter is not always static - it's an object, and you can manipulate it any way you'd like.
I saw it coming... I didn't mean "static" in C++ terms. What I meant is that filters have no clue on what they are filtering. The is_enabled function cannot make its decision on the context of execution. For example, you cannot make a filter that will discard all log messages from objects of class A and pass through messages from objects of class B. Unless they use different loggers, that is.
I can't believe you take this flexibility as a shortcoming. But please, explain how you see the concept of filtering.
Well, the example above illustrates it. Filtering is intended to selectively discard the unneeded information from logs. The criteria of such selection can be very complex and change during run time. For example, I run an ftp server and I want to see how much connections come from France. I'd like to be able to configure my server log filtering (or statistics) to log all connections from french IPs to a separate file. I don't see how I would implement it with your concept of filtering.
* I don't see the reason of all these macros for declaring and defining logs, tags, etc. I would rather prefer to see functions or objects forward declarations instead.
http://torjo.com/log2/doc/html/defining_your_logger_filter.html#declare_defi... http://torjo.com/log2/doc/html/defining_your_logger_filter.html
I've read it and I'm not convinced.
Anyway, I don't like functions like this, they are always a source of mistakes.
And what would the alternative be?
The logger gets initialized after at least one destination is added. But you didn't answer, will I be able to modify the set of destinations in run time?
* I see that there is no easy way to use once-initialized logs in module A (exe or dll) in another module B.
I really don't get what you're trying to say here. The dll_and_exe just shows that from EXE, you can use logs you define your own (from the EXE), and logs from another module - the DLL.
What code duplication are you talking about?
The log.cpp file in both exe and dll. What I want is: 1. Initialize logging somewhere in the beginning of the execution. For example, in the main. 2. Use the initialized loggers everywhere in the app - in exe itself and other dlls that may have not a slightest idea of where and how these logs are actually stored.
* The most critical note, from my point of view. I didn't find any trace of attributes or something like that. The library focuses on processing
Because you didn't look - it's tags, and it's way more powerful than what you have. http://torjo.com/log2/doc/html/namespaceboost_1_1logging_1_1tag.html
Been there. As far as I understood, it's yet an other way make formatting more flexible, and I didn't figure out when do I actually need it.
Why? Because your "attributes" are always there, and are fixed. Not to say that turning them off can be a pain.
Well, I want them to be there. Moreover, sometimes I want them to be everywhere the execution comes, whether it be another function, class or module. This is a powerful way of execution trace markup.
Tags, on the other hand, are flexible - you can add your own tags (tag classes), and adding tags or turning them off is just one or two lines of code.
That's fine, but what can I do with tags except to format them into the log message? Can I use them in filters? Can I use them to automatically generate values (for example, automatically log an actual count of objects of class A)?
* The design is not OOP-friendly. I can't create a logger object that is specific for a request that my application is currently processing (or
.... initialize it
That's the point. Since loggers also contain formatters and writers I would have to duplicate their initialization in each class if I want to have an object-specific logger. That's a burden, not to mention the performance loss.
can I?). This might be very useful if I wanted to log the context of execution (the request parameters, again, as attributes or a similar feature) in each log record. One can try to emulate such feature with a
Please elaborate on what you're trying to do here. It's way too vague...
Remember the ftp server example above? If I had a class that would represent an ftp connection (say, CFTPConnection), I would prefer to have a separate logger in each instance of this class. Why? Because then I could add connection-specific attributes to it, such as client IP, current state (idle, downloading, uploading, etc.)., connection duration, whatever. All this information would be implicitly available while I merely log at some checkpoints ("Requested file X", "Transmission complete"...). I can't do that with your solution.
* The "optimize" namespace and "favor" stuff is a questionable. IMO, the code should not contain things like this. Why would I chose to use non-optimized features? Because optimized ones lack some features or
Oh boy. The "favor"
I'm not against optimizations. If you feel these tools may provide benefits - fine. If there is a string type that performs better than STL but has restriction (honestly, who would need to optimize this, especially if you employ in-place formatting?), ok, place it into tools subdirectory. But please, don't call it optimized_string or optimized::string. Call it fixed_string, limited_string or preallocated_string if it limits the maximum buffer size. If you provide favor::speed, at least tell me what does it mean. Or better - make the name mean something. E.g. use_record_caching or whatever does it actually mean.
* Maybe a compilable configuration should be provided to reduce user's code build times. This would be a better solution than to depend on a macro that has different default values in release and debug.
Like BOOST_LOG_COMPILE_FAST_ON, BOOST_LOG_COMPILE_FAST_OFF? Which are already there
Exactly. I was saying that a precompiled library would be a better way than these macros.
* A better header segregation is needed. And it would be good to see in docs what I have to include to use each component of the library.
You mean: http://torjo.com/log2/doc/html/headers_to_include.html
Not exactly. What I meant is that on each component's description page there should be an appropriate #include. But I guess this info is available on the reference page. About a better header segregation. What I meant is that if I need logger, would have to include logger.hpp, if I need tag::file_line, I would include tag/file_line.hpp, etc.
* I can see that filtering may unnecessarilly block threads. That could be fixed with read/write mutexes.
What? Please explain.
detail/filter.hpp:146. ts::is_enabled acquires a scoped lock of a mutex. The function only reads the value, but I assume it may read it in multiple threads, doesn't it? If so, you could have used r/w mutex and acquire a read lock which doesn't block if there is a write lock.
* Use __LINE__ with care when compiling on MSVC 7.0+ with /ZI. You may run into problems when forming unique variable names. Use __COUNTER__ instead in such cases.
I think I have. Please give me an example of where I used this badly.
detail/scoped_log.hpp. BTW, it is used with BOOST_LOG_CONCATENATE, which I couldn't find anywhere but this file.
* Don't count on that pthread_t is ostreamable. Some platforms define it as a fundamental type, some as a structure. It may well be a char* which will most likely cause crashes in your code.
Thanks, didn't know that. What do you suggest? Dumping it as a (void*) pointer?
You might try to detect in compile time whether it is integral, pointer or structure. The first case is straightforward, in second you could cast it to uintptr_t, the last one is trickier. I guess the most portable way would be to dump it as sequence of sizeof(pthread_t) bytes (in hex, that is).
* What is your evaluation of the potential usefulness of the library?
I have a dual feeling about this. On one hand it provides a good set of features to implement logging in a relatively small and simple application. On the other hand, would I bother using it instead std::cout? Maybe.
I would say it would work on any size application. Why just small and simple application?
The reasons I have noted above. The large application usually consists of many modules and generates more logs. I have identified inconveniences in both support of modules and filtering logs.
* Do you think the library should be accepted as a Boost library?
No, at least at its current shape. I expect something different and more elaborate from the logging library. I believe that logging is not only writing strings into a file but it is an event reporting system. The library should be flexible enough to be able to support statistics gathering and alarming. The library should provide a flexible filtering
Please explain why my lib doesn't allow for statistics gathering and alarming.
Because your solution is geared to processing of strings. And statistics and alarming involve pieces of the application data, such as number of requests processed, amount of data transmitted, number of active connections, failure code and parameters, etc.
mechanism based on attributes of the logging records. I cannot see this
flexible filtering - why isn't this flexible enough? What do you want that is not in there?
See above.