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:

 > 1. zone_entry.h
 > * Suggest to remove "using namespace std;" from .h file and put 'std'
 prefix to related type.
 Good point - done.

 > * Suggest to change !ZoneEntry's ctor from
 > !ZoneEntry(AbstractRRset* nsrrset, vector<AbstractRRSet*> additional);
 > to
 > !ZoneEntry(AbstractRRset* nsrrset, const vector<AbstractRRSet*>&
 additional);
 > To avoid additional copying.
 Whoops, missed the "&" - done.

 > * Suggest to change the member variable of nameserver_ to nameservers_
 to indicate that it may contain multiple items.
 Done.

 > 2. nsas_types.h
 > * Suggest to change !NasAddress to !NsasAddress to keep the acronyms
 consistent.
 Done.

 > 3. nameserver_entry.h
 > * Suggest to remove "using namespace" statements from header file.
 Done.

 > * Suggest to add "const" to the input pointer variable:
 > !NameserverEntry(AbstractRRset* v4Set, AbstractRRset* v6Set, time_t
 curtime = 0);
 > to
 > !NameserverEntry(const AbstractRRset* v4Set, const AbstractRRset* v6Set,
 time_t curtime = 0);
 Done.

 > * Suggest to add a "virtual destructor" to !NameServer class.
 > See. http://www.parashift.com/c++-faq-lite/virtual-
 functions.html#faq-20.7
 Done.

 > * Suggest to add "inline" indicator to the small "accessor" functions.
 > I'm not sure whether the compiler will inline the code automatically,
 but add "inline" should do no harm.
 My understanding is that declaring the body of the code in the class
 definition is an implicit inline request, so the "inline" keyword is not
 needed here.

 > * For class of !AddressSelection, what is the type lf "result_type" ?
 > A little confused.
 bool.  This is a data type from the binary_function class, the others
 being "first_argument_type" and "second_argument_type".  They are typedefs
 to the template argument parameters, and using them means that the
 implementation of the function does not have to be altered if the template
 arguments are changed.  In this case though, the function is sufficiently
 simple and so unlikely to be altered that the advantage of result_type is
 outweighed by its obscurity.  I've changed it to bool.

 Also, I've rearranged the boolean expression to make it clearer.

 > * Suggest to change "class_" of class !ZoneEntry to "classCode_" to keep
 consistent with !NameserverEntry.
 Good suggestion - done.

 > 4. address_entry.h
 > * Suggest to move declaration of "UNREACHABLE" to "private" section,
 since it is used only internally.
 True.  However, it's been set public so that the internal operation can be
 tested.

 > * Suggest to initialize UNREACHABLE in the header file
 > * static const uint32_t UNREACHABLE =UINT32_MAX;  ///< RTT indicating
 unreachable address
 I've left it in the .cc file because of the need to define the macro
 !__STD_LIMIT_MACROS before including stdint.h in order to get UINT32_MAX
 defined.  Isolating it in the .cc file avoids any problems with this
 additional macro definition.

 > (lru_list.h)
 > BTW, I'm a little confused by the comments:
 > /// as list.size() may take some time if the list is big.
 > In my understanding, the std::list should also keep a separate count_
 internally, so std::list::size() should also be O(1)
 I'd thought that as well, but Scott Meyers in "Effective STL" (ISBN
 978-0-201-74962-5) suggests that in some implementations, this may not be
 the case.  The problem is the splice() call - knowing how many elements
 are being spliced into a list requires counting those elements, which can
 only be done by a traversal of the range being spliced; this slows down
 the operation.  So he suggests that some implementations, in a bid to keep
 splice() fast, may not keep a running count of the list size.  (This is
 actually discussed in "item 4", an article where he advocates the use of
 list.empty() in preference to (list.size() == 0).)

 It may be an unnecessary complication but as it is just an
 increment/decrement of a counter, I suggest we leave it it for now.

 > * Suggest to change
 > virtual uint32_t size() {
 > to
 > virtual uint32_t size() const  {
 Done.

 > * Since !LruList is also a virtual class, so I suggest to add a "Virtual
 Destructor" to it.
 Done.

 > 5. hash.h
 > * Shall this be shared by other component? If so, suggest to move it to
 a more common library such as utility,   otherwise change its name to
 nsas_hash.h
 Probably not, although I think there are some parts of the code that may
 well be more general.  I suggest we stay as we are and if it turns out
 that some code proves more generally useful, we'll move them at that time.

 > * Suggest to "const" to the operator() and mapLower() function, since
 they do not modify any member variable
 >virtual uint32_t operator()(const char* key, uint32_t keylen, bool
 ignorecase = true) const;
 > unsigned char mapLower(unsigned char inchar) const {
 Done.

 > * suggest to remove "using namespace std" declaration in header file
 Done.


 Good review, thank you. These changes have been committed to the branch in
 r3372.

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