BIND 10 #2355: DHCP v4 and v6 parsers' unification should be considered

[BIND 10 Development]

#2355: DHCP v4 and v6 parsers' unification should be considered
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tmark
                Type:  defect        |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcpconf      |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130509
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tmark


Comment:

 Reviewed commit 53ceb71e36d310a2a4cd5fef7ae9fa5d410f078a.

 '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 Please start comments with a capital letter.

 Subnet4ConfigParser constructor: all parameters (even unused ones) should
 be listed in the header.

 Subnet4ConfigParser::createSubnet4ConfigParser: in the "else if" block,
 the "else" should be on the same line as the preceding closing brace, i.e.
 "} else if (...) {"

 Subnet4ConfigParser::createSubnet4ConfigParser: please extend the @return
 comment in the header to note that the caller is responsible for deleting
 the returned parser once it is no longer needed.

 configureDhcp4Server: there are several debug statements outputting to
 cout here.

 Dhcp4OptionDataParser methods: "return" statements should enclose the
 returned value in parentheses.

 createGlobalDhcp4ConfigParser: in the "else if" block, the "else" should
 be on the same line as the preceding closing brace, i.e. "} else if (...)
 {"


 '''src/bin/dhcp4/config_parser.h'''[[BR]]
 Dhcp4OptionDataParser constructor: all arguments should be documented.

 Dhcp4OptionDataParser constructor and "factory" method: attach the
 asterisk of a pointer declaration to the data type rather than the
 variable (i.e. use "type* var" rather than "type *var" - see below).

 Dhcp4OptionDataParser: Do you need to define a private default constructor
 if a non-default one has been defined?

 '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 Please start comments with a capital letter.

 !PoolParser constructors: arguments (even unused arguments) should be
 described in the header.

 Subnet6ConfigParser constructor: arguments (even unused arguments) should
 be described in the header.

 Subnet6ConfigParser::createSubnet6ConfigParser: in the "else if" block,
 the "else" should be on the same line as the preceding closing brace, i.e.
 "} else if (...) {"

 Subnet6ConfigParser::createSubnet6ConfigParser: please extend the @return
 comment in the header to note that the caller is responsible for deleting
 the returned parser once it is no longer needed.

 Dhcp6OptionDataParser methods: "return" statements should enclose the
 returned value in parentheses.

 Subnet6ListConfigParser constructor: arguments (even unused arguments)
 should be described in the header.

 Dhcp6OptionDataParser methods: "return" statements should enclose the
 returned value in parentheses.

 createGlobalDhcpConfigParser: in the "else if" block, the "else" should be
 on the same line as the preceding closing brace, i.e. "} else if (...) {"

 createGlobalDhcpConfigParser: for symmetry, should consider renaming this
 createGlobalDhcp6ConfigParser.


 '''src/bin/dhcp6/config_parser.h'''[[BR]]
 Try breaking lines at 80 columns - there are only a couple of places where
 this is violated.

 Dhcp6OptionDataParser constructor and "factory" method: attach the
 asterisk of a pointer declaration to the data type rather than the
 variable (i.e. use "type* var" rather than "type *var" - see below).

 Dhcp6OptionDataParser constructor: all arguments should be documented.

 Dhcp6OptionDataParser: Do you need to define a private default constructor
 if a non-default one has been defined?


 '''src/lib/dhcpsrv/dhcp_parsers.h'''[[BR]]
 !BooleanParser, !Uint32Parser and !StringParser appear to be identical
 apart from the data type of the storage_ member (and !XxxStorage argument
 to the constructor).  They could be replaced by a template class plus a
 set of typedefs.

 Multiple declarations: the de-facto standard when declaring pointers is to
 attach the asterisk to the data type, e.g. use
 {{{
 Uint32Storage* storage;
 }}}
 instead of the C-standard:
 {{{
 Uint32Storage *storage;
 }}}
 (This fails if you declare multiple variables on the same line, but our
 standard is not to do that.)  This happens at various points in the code.

 !XxxParser constructor: second argument should be described in the header.
 Since a raw pointer is being passed, the description should also describe
 who is responsible for destropying the pointed-to object and whether the
 storage object has to remain in scope while an object derived from the
 parser class is active.

 !OptionDataParser constructor: all parameters must be described in the
 header.

 OptionDataParser::commit: suggest wrapping the header comments at 80
 characters.
 src/lib/dhcpsrv/dhcp_parsers.h

 Multiple classes: if a non-default constructor has been defined, do you
 need to define the default constructor? Isn't that only added if no
 constructor is present?



 '''src/lib/dhcpsrv/dhcp_parsers.cc'''[[BR]]
 !XxxParser constructor: as the logic of checking whether the parameter
 name is empty is the same in each of the specialisations, putting the
 check into a private method of the template class would avoid the
 repetition of the error message in the code.

 XxxParser::commit(): these are all the same, so can be in the general
 template class.

 XxxParser::build(): these can be specialisations of the template.

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