On Oct 7, 2004, at 2:51 PM, Christopher Smith wrote:
Curt Arnold wrote: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.
The externally observed behavior should be consistent at least.
1) You pay the price of constructing std::basic_stream regardless of whether logging is turned on.
Do you have any metrics on the cost of that? I don't think there is a way to avoid that unless you make the log stream not derived from the basic_stream and I think that it would be undesirable to make logstream kind of like, but not exactly, a basic_stream.
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).
This is a convenience API and adds no dependencies on standard streams unless the header is included. Nothing precludes similar templates based on pre-standard or nicer stream implementations. It may be worthwhile to remove iostream dependency for log4cxx as a whole, but I don't think it is worth a huge effort to attempt to support any other stream frameworks within log4cxx itself.
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__); }
Each insertion operator will check a cached boolean value containing the result of IsEnabledFor(), but that should be very cheap. You could probably only update the value at change of level and not a and shave the call to IsEnabledFor. However, if you still called forcedLog() then you could have log events still being fired after a configuration change. If you don't call forcedLog, then you still have the IsEnabledFor call within the Logger.log() method. I guess you could update isEnabled only if the value was true which would allow you to stop an existing stream for throwing off messages, but an disabled stream would stay disabled until the level was changed.
4) The whole LOG4CXX_STREAM_DEFINE_INSERTION seems much uglier than using a templated method.
I tried a templated method, but got a lot of ambiguity messages and couldn't come up with anything more elegant at short notice. You would need some sort of filter, since you would still like insertions that modify the state of the stream, like "<< scientific ", to still be effective regardless of the current level.
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.
I'll take a look at that.
