BIND 10 #356: Resolver address database

[BIND 10 Development]

#356: Resolver address database
-------------------------------+--------------------------------------------
      Reporter:  stephen       |        Owner:  ocean    
          Type:  enhancement   |       Status:  reviewing
      Priority:  major         |    Milestone:           
     Component:  Unclassified  |   Resolution:           
      Keywords:                |    Sensitive:  0        
Estimatedhours:  0.0           |        Hours:  0        
      Billable:  1             |   Totalhours:  0        
      Internal:  0             |  
-------------------------------+--------------------------------------------
Changes (by stephen):

  * owner:  stephen => ocean


Comment:

 Reviewed differences between latest version at time of writing (r3706) and
 the last-reviewed revision (r3533)

 '''src/lib/nsas/random_number_generator.h'''[[BR]]
 Need to {{{#include <math.h>}}} (on Ubuntu at least) to get the
 declaration of {{{fabs()}}}.

 ''WeightedRandomIntegerGenerator::isProbabilitiesValid''[[BR]]
 Although the compiler will do the appropriate conversions, I suggest using
 literals that are doubles (1.0, 0.0) instead of integers to emphasise the
 fact that we are comparing floating-point numbers.

 ''WeightedRandomIntegerGenerator::isProbabilitiesValid''[[BR]]
 It is clearer to combine the two range checks in one statement, i.e.
 {{{
 if ((*it < 0.0) || (*it > 1.0)) {
     return false;
 }
 }}}

 '''src/lib/nsas/tests/random_number_generator_unittest.cc'''[[BR]]
 Some of the tests use the ASSERT_DEATH macro.  This relies on the assert()
 macro in the constructor to call isProbabilitiesValid() and, if it returns
 false, to call abort().  However, if the code is compiled with NDEBUG set,
 assert() is a no-op and the test fails.


 '''src/lib/nsas/tests/Makefile.am'''[[BR]]
 Should include a dependency on random_number_generator.h

 '''src/lib/nsas/nameserver_address.h'''[[BR]]
 The constructor contains the line:
 {{{
  if(!ns_.get()) isc_throw(NullNameserverEntryPointer, "NULL
 NameserverEntry pointer.");
 }}}
 The [wiki:CodingGuidelines coding guidelines] request that braces be used
 even for a single-line "if" block.

 '''src/lib/nsas/nameserver_entry.h'''[[BR]]
 '''src/lib/nsas/nameserver_entry.cc'''[[BR]]

 ''NameserverEntry::getAddress()''[[BR]]
 The first argument  must be a shared_ptr pointing to "this".  As it is
 only used in constructing a !NameserverAddress object (which is then
 copied over the passed !NameserverAddress object), the argument seems
 redundant.  Suggest either:
  a. the passed-in !NameserverAddress object could be partially constructed
 using the current !NameserverEntry before calling this method. (The
 assert() call in this method would be kept to ensure that the argument is
 compatible with this object.)
  b. Derive !NamserverEntry (or perhaps its base class !NsasEntry, so
 avoiding multiple inheritance) from boost::enable_shared_from_this, which
 will allow the construction of a shared_ptr for "this" within the method.

 ''NameserverEntry::updateAddressRTTAtIndex()''[[BR]]
 The [wiki:CodingGuidelines coding guidelines] request that braces be used
 for all "if" and "else" clauses.

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