BIND 10 #901: Alter logging interface to use positional formatting for message arguments

[BIND 10 Development]

#901: Alter logging interface to use positional formatting for message arguments
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => vorner


Comment:

 '''src/lib/asiodns/io_fetch.cc'''[[BR]]
 The comment starting "Although Logger::debug checks..." can be removed as
 the detail about isDebugEnabled() is now hidden inside the LOG_DEBUG
 macro.

 It is probably worth defining constants for the debug levels here. (We are
 doing it elsewhere, so was might as well take the opportunity to do it
 here, although it is not strictly part of the ticket.)

 '''src/lib/log/log_formatter.{cc,h}'''[[BR]]
 Is there a need to have a copy constructor (or assignment operator) for
 the Formatter object?  arg() could be declared "Formatter&" (returning
 *this) and could increment nextPlaceholder_ when called.  It would avoid
 multiple objects being constructed and destroyed. (It would also allow
 message_ to be stored in the class itself.)

 replacePlaceholder(): I don't think that it should add output if a
 placeholder is not found - there may be a valid reason for a missing
 placeholder, especially if the message is translated into multiple
 languages.  A rather contrived example (the first that came into my head)
 is "%1 sheep" in English and "%1 mouton%2" in French, called with
 {{{
 .arg(n).arg((n == 1) ? "" : "s")
 }}}

 replacePlaceholder(): it may be more flexible to replace all instances of
 a placeholder in a string rather than the first one.  (To avoid recursion,
 the second and subsequent searches for placeholders would need to start
 beyond the replaced text.)

 replacePlaceholder(): is there any reason why this is not a private method
 of the class?

 '''src/lib/log/logger_impl.cc'''[[BR]]
 Should really include <iomanip> in logger_impl.cc to define "endl".  (It's
 not clear in which header file <iomanip> is actually included, and this
 could cause problems if it were subsequently removed from that file.)


 '''src/lib/log/macros.h'''[[BR]]
 In some cases in C/C++, the language requires a statement but the logic of
 the program does not.  In this case a null statement - the semi-colon by
 itself - is used, e.g.
 {{{
 for (len = 0; str[len]; ++len)
    ;
 }}}
 Braces are used to surround a block of statements, but I've not managed to
 find anything that explicitly states that it is valid for them to surround
 an empty block.  Would it be safer for the braces to include a single
 semi-colon?

 It might be useful to include macros.h in logger.h; that way the macros
 are available whenever the logger is available.

 '''src/lib/log/tests/log_formatter_unittest.cc'''[[BR]]
 In the "active" test, the ASSERT_LE test seems superfluous since the next
 test is EXPECT_EQ on the same variable.

 Should add a test to ensure that substituting "%1" with "%1" does not
 cause an endless loop.

 '''!ChangeLog entry'''[[BR]]
 The logging code has been provided in a previous release, so I think there
 should be a !ChangeLog entry for this.

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