From: Tomek Mrugalski <tomasz@isc.org>
Date: Wed, 28 Dec 2016 16:07:45 +0000 (+0100)
Subject: [5021] Changes after review:
X-Git-Tag: trac5101_base~1^2~2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=8e2c1545016f316adfa96127f68bac7f6b8eedc2;p=thirdparty%2Fkea.git

[5021] Changes after review:
 - using const_pointer_cast in dhcp{4,6}/json_config_parser.cc
 - removed obsolete comment from dhcp_parsers_unittest.cc
 - updated simple_parser.h comments
 - added include <vector> in simple_parser.h
---

diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc
index bcfad010e8..3344a4b895 100644
--- a/src/bin/dhcp4/json_config_parser.cc
+++ b/src/bin/dhcp4/json_config_parser.cc
@@ -566,7 +566,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
         // This is a way to convert ConstElementPtr to ElementPtr.
         // We need a config that can be edited, because we will insert
         // default values and will insert derived values as well.
-        ElementPtr mutable_cfg = Element::getMutableMap(config_set);
+        ElementPtr mutable_cfg = const_pointer_cast<Element>(config_set);
 
         // Set all default values if not specified by the user.
         SimpleParser4::setAllDefaults(mutable_cfg);
diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc
index 98e1c081b0..cd1350deae 100644
--- a/src/bin/dhcp6/json_config_parser.cc
+++ b/src/bin/dhcp6/json_config_parser.cc
@@ -840,7 +840,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
         // This is a way to convert ConstElementPtr to ElementPtr.
         // We need a config that can be edited, because we will insert
         // default values and will insert derived values as well.
-        ElementPtr mutable_cfg = Element::getMutableMap(config_set);
+        ElementPtr mutable_cfg = const_pointer_cast<Element>(config_set);
 
         SimpleParser6::setAllDefaults(mutable_cfg);
 
diff --git a/src/lib/cc/simple_parser.cc b/src/lib/cc/simple_parser.cc
index aeaeb469bb..21444b75e7 100644
--- a/src/lib/cc/simple_parser.cc
+++ b/src/lib/cc/simple_parser.cc
@@ -123,7 +123,7 @@ size_t SimpleParser::setDefaults(isc::data::ElementPtr scope,
 
         // ... and insert it into the provided Element tree.
         scope->set(def_value.name_, x);
-        cnt++;
+        ++cnt;
     }
 
     return (cnt);
diff --git a/src/lib/cc/simple_parser.h b/src/lib/cc/simple_parser.h
index 2911c64900..31bc835a7c 100644
--- a/src/lib/cc/simple_parser.h
+++ b/src/lib/cc/simple_parser.h
@@ -8,6 +8,7 @@
 #define SIMPLE_PARSER_H
 
 #include <cc/data.h>
+#include <vector>
 #include <string>
 #include <stdint.h>
 
@@ -33,17 +34,20 @@ typedef std::vector<std::string> ParamsList;
 
 /// @brief A simple parser
 ///
-/// This class is intended to be a simpler replacement for
-/// @ref isc::dhcp::DhcpConfigParser.
+/// This class is intended to be a simpler replacement for @ref
+/// isc::dhcp::DhcpConfigParser. This class has been initially created to
+/// facilitate DHCPv4 and DHCPv6 servers' configuration parsing. Thus examples
+/// provided herein are related to DHCP configuration. Nevertheless, this is a
+/// generic class to be used in other modules too.
+///
 /// The simplification comes from several factors:
 /// - no build/commit nonsense. There's a single step:
 ///   CfgStorage parse(ConstElementPtr json)
 ///   that converts JSON configuration into an object and returns it.
-/// - almost no state kept. The only state kept in most cases is whether the
-///   parsing is done in v4 or v6 context. This greatly simplifies the
-///   parsers (no contexts, no child parsers list, no separate storage for
-///   uint32, strings etc. In fact, there's so little state kept, that this
-///   parser is mostly a collection of static methods.
+/// - no state kept. This greatly simplifies the parsers (no contexts, no child
+///   parsers list, no separate storage for uint32, strings etc. In fact,
+///   this base class is purely static. However, some derived classes may store
+///   some state. Implementors are advised to store as little state as possible.
 /// - no optional parameters (all are mandatory). This simplifies the parser,
 ///   but introduces a new step before parsing where we insert the default
 ///   values into client configuration before parsing. This is actually a good
@@ -98,14 +102,15 @@ class SimpleParser {
 
     /// @brief Utility method that returns position of an element
     ///
-    /// It's mostly useful for logging.
+    /// It's mostly useful for logging. When any necessary parameter is
+    /// missing (either parent is null or it doesn't contain specified
+    /// name) ZERO_POSITION is returned.
     ///
     /// @param name position of that element will be returned
     /// @param parent parent element (optional)
     /// @return position of the element specified.
     static const data::Element::Position&
-    getPosition(const std::string& name, const data::ConstElementPtr parent =
-                data::ConstElementPtr());
+    getPosition(const std::string& name, const data::ConstElementPtr parent);
 
  protected:
 
diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
index a29e792e8d..566be20b64 100644
--- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
+++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
@@ -428,7 +428,6 @@ public:
                           const SimpleDefaults& option_def_defaults) {
         size_t cnt = 0;
         // Set global defaults first.
-        /// @todo: Uncomment as part of the ticket 5019 work.
         cnt = SimpleParser::setDefaults(global, global_defaults);
 
         // Now set option defintion defaults for each specified option definition