BIND 10 #2634: The DhcpConfigParser::getParam should be abstracted to a separate class.

[BIND 10 Development]

#2634: The DhcpConfigParser::getParam should be abstracted to a separate class.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  low           |                       Status:
           Component:  dhcpconf      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130411
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => stephen


Comment:

 Replying to [comment:4 stephen]:
 > Reviewed commit ff8f84b695de0e5350af325c675ab2bdbab79cc0
 >
 > '''General'''[[BR]]
 > There is no need to attach the output of unit tests to the ticket.

 Duly noted.  Will exclude from future tickets.
 >
 > '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 > Why did one of the changes move the include of dhcp4/config_parser.h
 lower down the list of includes (originally the include files were in
 alphabetical order)?
 >
 > The include of dhcpsrv/dhcp_config_parser.h is commented out.

 Didn't mean to commit this change.

 >
 > createSubnet - please start comments with a capital letter ("Rethrow
 with precise error").
 >

 Done.

 > '''src/bin/dhcp4/tests/config_parser_unittest.cc'''[[BR]]
 > Dhcp4ParserTest::checkGlobalUint32: commented-out declarations have been
 left in.  They should be removed.

 Done.  I need to be more diligent with my pre-commit "review".

 >
 > '''src/lib/dhcpsrv/dhcp_config_parser.h'''[[BR]]
 > The file "stdint.h" is included.  The C++ version of this is "cstdint"

 Here, I was following suit.  "stdint.h" is included all over our source
 tree.  I attempted to use <cstdint> but it does not compile cleanly under
 OS-X (not found) or Debian:

 "/usr/include/c++/4.4/c++0x_warning.h:31:2: error: #error This file
 requires compiler and library support for the upcoming ISO C++ standard,
 C++0x. This support is currently experimental, and must be enabled with
 the -std=c++0x or -std=gnu++0x compiler options."

 To use it would require build option changes.  I recommend leaving it at
 stdint.h.

 >
 > Typo in header of !ValueStorage class - "@tparam".
 >
 > setParam - typos in header: "parmater", "paramater". (Second typo also
 occurs in the description of the retur
 n value in getParam.)
 >

 Done.
 > setParam takes its parameters by value.  If these are complex objects
 (like strings), that will cause a local
  variable to be created via a copy constructor then deleted.  I suggest
 that they are passed by reference.
 >
 Done.

 > Suggest adding a blank line after the end of setParam to separate it
 from the next method. (This is done else
 where.)
 >
 > getParam - text in exception should start with a capital letter
 ("Missing parameter").
 >
 > getParam - is there a need for the intermediate statement setting
 "value"?  {{{return (param->second)}}} seem
 s reasonably clear.
 Done.
 >
 > delParam - pass parameter by reference?
 >
 Done.

 > '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''[[BR]]
 > (All comments apply to the new tests)
 >
 > Please start comments with capital letters.
 >
 > What is the "them" in the comments?

 Done.

 >
 > The tests should also check:
 > * Trying to get something that never existed in the storage throws an
 exception. (The current test only checks that getting something that has
 been deleted throws an exception.)
 > * The expected behaviour of deleting something that does not exist.
 >
 > The failure of deleting one of the parameters can be done via an
 EXPECT_THROW.  If that test fails, the subsequent tests can still be
 carried out. (The same goes for the test after emptying the list.)
 >

 Adding cases to:
  1.  Verify that asking for something that never existed throws and
 exception.
  2.  Verify that deleteing  something that never existed does NOT throw.

 For all three storage tests.

 > I'm surprised it's not caught by the compiler, but in the testing of
 Uint32Storage, one of the test value is negative.
 >

 Changed negative test value to positive.

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