BIND 10 #558: Disable log4cxx dependency in logging API

[BIND 10 Development]

#558: Disable log4cxx dependency in logging API
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  stephen
                     Type:  defect   |               Status:  reviewing
                 Priority:  major    |            Milestone:  R-Team-
                Component:  logging  |  Sprint-20110208
                 Keywords:           |           Resolution:
Estimated Number of Hours:  2.0      |            Sensitive:  0
                Billable?:  1        |  Add Hours to Ticket:  0
                Internal?:  0        |          Total Hours:  0
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => stephen


Comment:

 I'm unsure what the goal (and/or the next step) is now; I had the
 following comments, until I realized that the whole purpose was to really
 remove log4cxx use, but only 'temporarily', as it says in the ticket, so
 what to do about this?

 {{{
 lib/log/Makefile.am (also) contains a few commented-out includes (for
 LOG4CXX) that can probably be removed now (as well as the commented-out
 part of configure.ac)

 is logger_impl_log4cxx.h still used (or cc for that matter)?
 }}}


 The pimpl use itself (in logger_impl.h) seems a bit off, isn't the idea
 not to actually *define* the xxxImpl class in the headers, but to only
 name it and let the .cc define it?

 Minor points for the code:

 is lib/log/tests/xdebuglevel_unittest still used? (it got commented out in
 the makefile) (perhaps also related to above)

 Really minor (and i mean MINOR, one would almost call it trivial) naming
 point:
 LoggerImpl* getLoggerptr() {
 It's private and all, just wanted to mention that we tend to capitalize
 'ptr' in the rest of our code :)
 (although, come to think of it, we use the Ptr postfix usually to name a
 shared_ptr)

 You might want to have the generator include some comments before that
 initializer and instantiator as to why they are necessary, and why they
 have such a weird name (and/or why that doesn't matter).

 LoggerImpl, lots of lexical casts there (because of the change of
 MessageID I guess), which seem unnecessary in this context, why not pass
 the ident and text as separate arguments to output(), and have that
 function operator<<() them separately?


 I am not sure I oppose your suggestion at the bottom of #559, btw, perhaps
 more of this actually warrants a focused phone discussion?

 Apart from this, the code looks ok, btw :) (though perhaps Jinmei wants to
 have a quick look at the changes for the init problem)

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