BIND 10 #899: Implement logging using Log4cplus

[BIND 10 Development]

#899: Implement logging using Log4cplus
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110531
                  Component:         |            Resolution:
  logging                            |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
  logging                            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => vorner


Comment:

 > Why is the destructor of !DerivedLogger explicitly mentioned and
 virtual? As this class has no virtual functions (except for the inherited
 ones), there's no need for one, the default will be virtual, because the
 destructor of parent class is already virtual. Not that it would be wrong,
 it just surprised me.
 No good reason (other than !DerivedLogger was copied from elsewhere), it's
 been removed.

 > I don't like including the linker flags everywhere. Is it really needed?
 Because I think the whole purpose of our liblog is to hide the internal
 implementation.
 Well caught.  It should have been in the Makefile.am for liblog, not
 everywhere else!  Corrected.

 > This bit looks strange...
 Incomplete last sentence removed - it now reads just "Minimum/maximum
 debug levels".

 > Isn't a class with only static methods a little bit misleading? Such
 class is more like namespace, isn't it? (Not that it would produce
 different code, in the end)
 Possibly, but I wanted to group the methods together (the init() call
 relates to two of those methods) and indicate that they are
 implementation-specific functions.  Putting them in the general isc::log
 namespace loses that grouping, but I didn't want to create a new namespace
 under it.  A class name ending in Impl provides the grouping and conveys
 the information that they are implementation-specific (so are related to
 other xxxImpl classes).

 > specify exact directory of log4cplus library and headers ‒ this message
 looks strange to me. Is it possible to specify inexact directory? And it
 looks like they should live in one directory, but one is in lib, one in
 include, I really wondered which one of them you mean for a short while.
 Wouldn't „installation prefix of log4cplus“ be better?
 configure.ac also includes the messages "specify exact directory of Botan
 library" and "specify exact directory for Boost headers", so I guess this
 one is consistent.

 > This FIXME in Formatter::arg was wrong, because the underlying replace
 did replace all occurrences. However, what you describe isn't the biggest
 problem (that is, actually, quite expected output). If we use the original
 example, you would now get "42 42" (the first %1 gets replaced to %2 and
 then both %2 get replaced), which is by no means expected output (it is
 understandable once you get it, but it's surprising).
 I've updated the comment.

-- 
Ticket URL: <http://bind10.isc.org/ticket/899#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development