BIND 10 #2316: Extend "Subnet" with OptionCollection structure

[BIND 10 Development]

#2316: Extend "Subnet" with OptionCollection structure
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  marcin                             |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121018
  medium                             |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:  2318   |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 82c0737d3d63da1a1c2b001d0306a32a1e55f5f5

 '''src/lib/dhcp/subnet.cc'''[[BR]]
 Subnet::addOption(), Subnet{4,6}::validateOption(): Suggest passing the
 "option" argument by reference (instead of value).  As !OptionPtr is a
 boost::shared_ptr, this saves the overhead associated with the copying of
 the shared_ptr object on entry (including the incrementing of the
 reference count) and its destruction (and decrement of the reference
 count) on exit.

 Subnet{4,6}::validateOption(): as these methods don't alter the shared_ptr
 (nor the unerlying reference count), can the option argument be declared
 "const"?

 Subnet{4,6}::validateOption(): typo - "epected".

 Subnet{4,6}::validateOption(): it occurs to me that if, within option.h,
 explicit numeric values were to be assigned to the members of the
 "Universe" enum:
 {{{
 enum Universe {
     V4 = 4,
     V6 = 6
 };
 }}}
 ... then "validateOption()" could be put in the "Subnet" base class.  An
 extra argument for the universe expected would be used in the
 getUniverse() check, and the value of the argument, cast to an "int",
 would be used in the error message.

 '''src/lib/dhcp/subnet.h'''[[BR]]
 !OptioNDescriptor constructor: header needs "@param" tags to describe the
 options.

 !KeyFromKey header: not quite clear what is meant by "This class extracts
 one multi_index_container key with another key".  Does this mean that
 given one key, it returns a value that can then be sed as another key?

 !KeyFromKey: as the members are private and only methods public, this is
 probably better declared as a "class" instead of a "struct".

 !KeyFromKey: thanks for the indented structure and detailed description of
 each part of the "index_by" template: it made it much easier to
 understand.  Just one suggestion: the comments refer to indexes !#1 and
 !#2: is the "sequenced<>" construct index !#0?  If so, I suggest that the
 comment mention it.

 getOptions(): should add in the "@return" comment that the reference
 returned is valid only for as long as the Subnet object remains in
 existence.

 '''src/lib/dhcp/tests/subnet_unittest.cc'''[[BR]]
 addPersistentOption test: probably want an unequal split the number of
 persistent and non-persistent options.  When testing a 5/5 split, it is
 possible that the both the function returning the count of persistent
 options and the one returning the non-persistent count are in fact
 returning the persistent count (or non-persistent count).

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