BIND 10 #438: C++ Logging

[BIND 10 Development]

#438: C++ Logging
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  jelte
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110125
                Component:  logging  |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  5.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jelte


Comment:

 Latest commit: a0d8f67619db8737abda16b3ef9b1b731ab1bfa1

 > General:
 > Is the range of debug levels (0-99) based on anything specific? I shall
 have a hard time remembering more than,
 > say, 5 levels :)
 It's based on BIND-9.  I don't know where the levels are documented, but
 they go up to at least level 90 according to the O'Reilly book (chapter
 13).

 > In a few files, i noticed a space after the ! operator in conditionals,
 is this a style thing i  wasn't aware of
 > (we don't do that anywhere else)
 Personal quirk (I think it's easy to miss the "!" if it is concatenated
 with the variable).  But I've removed the spaces to comply with the usage
 in BIND-10.

 > Message.h:
 > is this file used or needed anywhere or is this leftover code?
 > (if so i have some comments, but i suspect the latter)
 ... and your suspicions would be right.  It's been removed.

 > filename.*:
 > we should add some tests for corner cases in the split function
 Added.  It also prompted me to add some more test cases for
 StrUtil::tokens() (which also splits a string up into component parts).

 > why is #ifdef WIN32 around normalize only in the header and not in the
 normalize calls in expandwith and useas?
 They were missed there, the #ifdef brackets have now been added. (See
 comment below about location of the #ifdef brackets for normalize()).

 > logger.h:
 > a bit of whitespace in the doxygen code would make it somewhat more
 readable
 Added.

 > ps. so now we have unfree'd memory on exit? once that problem gets fixed
 in log4cxx we should
 > consider an (optional) way to remove this behaviour, if we want to run
 memory checkers on our code
 If log4cxx gets fixed, we just add the appropriate "delete" statement to
 the destructor.  If we could work out some way of checking at run-time
 what version of log4cxx we are using, that check could be part of the
 destructor.

 > there are two getters for warning enabled; getWarnEnabled() and
 getWarningEnabled()
 I added two as there were two synonyms for warning: WARN and WARNING;
 there was also the synonym CRITICAL for FATAL.  On reflection, this could
 be seen as confusing, so the synonyms have been removed.  The categories
 (and all functions based on them) now use the words DEBUG, INFO, WARN,
 ERROR and FATAL, and these are the words output to the log.

 > btw, should we provide some hints as to when to use which level?
 The section "Severity Guidelines" has been added to
 src/lib/log/documentation.txt

 > Perhaps add an extra non-doxygen comment before the !LoggerPtr?* member
 in the Logger class that this is a pointer to
 > a Ptr because of the static/dynamic delete problem mentioned earlier?
 Added.

 > (btw, if we want to hide log4cxx completely we should not expose that
 !LoggerPtr? in logger.h)
 Good point.  However, rather than delay completion of this ticket or add
 additional work to the current sprint, I've created the low-priority task
 #529 for it.

 > logger.cpp:
 > According to our coding guidelines, we should have a newline after the
 return value of a function > definition
 Force of habit I'm afraid :-$  All occurrences have been changed to comply
 with the standard.

 > strutil.cc:
 > just thought of something, perhaps you can also to the if win32 here
 (and have the call simply
 > be a nop on non-win systems)
 I thought of that too, but that adds redundant call/returns.  Also, having
 just written the code I wanted to test it: on Unix that's easier if we
 don't have the code commented out with WIN32.  Finally, there ''might'' be
 circumstances where normalizeSlash() is required even when Windows is not
 involved.  For these reasons - plus the fact that it is only called in a
 couple of places - I've left it as-is (having commented out those calls
 mentioned earlier).

-- 
Ticket URL: <https://bind10.isc.org/ticket/438#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development