Some additional comments on the patch are available at the bug report, LOGCXX-18. The major difference is that this logstream is derived from std::basic_stream and can be passed to methods expecting that stream. The insertion operator will typically be short-circuited when the concrete type is known and the logger is disabled (at least that is the intent, haven't confirmed it). If the concrete type is lost (as when passed as a ostream&) or for custom insertion operations, the insertion operation will still be performed, but the message will be discarded.
Hmmmm.. Aside from the inconsistent behavior based on the concrete type, I see a number of short comings with this approach:
1) You pay the price of constructing std::basic_stream regardless of whether logging is turned on.
2) Because it's templated on char_traits instead of on the stream type, you are forced to always use std::basic_stringbuf. This is bad for folks who want to avoid iostreams (although I I can see where there isn't much chance of that working out anyway), but more importantly it's bad for those who might have a nicer stream implementation (say one that isn't built around fixed width characters).
3) Having parts of the code checking logger->isEnabledFor() and others checking isLoggerEnabled seems aesthetically unclean and seems to me to produce some undesirable behavior. Also, since isLoggerEnabled is not a const (so that it can be changed), you're going to end up with generated code that probably checks for things being enabled several times for each logging event. This means the following code will most likely be more optimal than using logstream:
if (logger->isEnabledFor(myLevel)) {
ostringstream buffer;
buffer << /* some stuff */;
logger->forcedLog(myLevel, buffer.str(), __FILE__, __LINE__);
}4) The whole LOG4CXX_STREAM_DEFINE_INSERTION seems much uglier than using a templated method.
BTW, are zero-length logs considered inherently bad?
The log events are raised when the stream is flushed. Inserting LOG4CXX_ENDMSG will set the location and flush the message. Changed the name from ENDL since no new-line is actually added.
Yeah, ENDL was bad idea in retrospect.
Most of my concerns are really at the level of design decisions, and I'm willing to bet you already thought of all the drawbacks I mentioned, so I won't push much for changes. However, I do think #2 is important. It would seem much cleaner to me to have logstream inherit from it's second template parameter, and I don't see any disadvantages to that approach.
--Chris
