From 5ab6bde1dbc0bdaacb392f6b20ab4c812e966def Mon Sep 17 00:00:00 2001
From: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Date: Mon, 4 Jun 2018 15:06:01 -0700
Subject: [PATCH] fully delete parameters (#489)

* allow ParameterValue class to be copy constructed with type not-set

* actually remove deleted parameters from the map
---
 .../node_interfaces/node_parameters.cpp       | 33 +++++++++++--------
 rclcpp/src/rclcpp/parameter_value.cpp         |  3 +-
 rclcpp/test/test_parameter.cpp                |  4 +--
 3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
index 698330f..fcc7c87 100644
--- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
+++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
@@ -88,27 +88,34 @@ NodeParameters::set_parameters_atomically(
   }
 
   for (auto p : parameters) {
-    if (parameters_.find(p.get_name()) == parameters_.end()) {
-      if (p.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET) {
+    if (p.get_type() == rclcpp::ParameterType::PARAMETER_NOT_SET) {
+      if (parameters_.find(p.get_name()) != parameters_.end()) {
+        // case: parameter was set before, and input is "NOT_SET"
+        // therefore we will erase the parameter from parameters_ later
+        parameter_event->deleted_parameters.push_back(p.to_parameter_msg());
+      }
+    } else {
+      if (parameters_.find(p.get_name()) == parameters_.end()) {
         // case: parameter not set before, and input is something other than "NOT_SET"
         parameter_event->new_parameters.push_back(p.to_parameter_msg());
+      } else {
+        // case: parameter was set before, and input is something other than "NOT_SET"
+        parameter_event->changed_parameters.push_back(p.to_parameter_msg());
       }
-    } else if (p.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET) {
-      // case: parameter was set before, and input is something other than "NOT_SET"
-      parameter_event->changed_parameters.push_back(p.to_parameter_msg());
-    } else {
-      // case: parameter was set before, and input is "NOT_SET"
-      // therefore we will "unset" the previously set parameter
-      // it is not necessary to erase the parameter from parameters_
-      // because the new value for this key (p.get_name()) will be a
-      // Parameter with type "NOT_SET"
-      parameter_event->deleted_parameters.push_back(p.to_parameter_msg());
+      tmp_map[p.get_name()] = p;
     }
-    tmp_map[p.get_name()] = p;
   }
   // std::map::insert will not overwrite elements, so we'll keep the new
   // ones and add only those that already exist in the Node's internal map
   tmp_map.insert(parameters_.begin(), parameters_.end());
+
+  // remove explicitly deleted parameters
+  for (auto p : parameters) {
+    if (p.get_type() == rclcpp::ParameterType::PARAMETER_NOT_SET) {
+      tmp_map.erase(p.get_name());
+    }
+  }
+
   std::swap(tmp_map, parameters_);
 
   events_publisher_->publish(parameter_event);
diff --git a/rclcpp/src/rclcpp/parameter_value.cpp b/rclcpp/src/rclcpp/parameter_value.cpp
index be3ecbe..44fd4c9 100644
--- a/rclcpp/src/rclcpp/parameter_value.cpp
+++ b/rclcpp/src/rclcpp/parameter_value.cpp
@@ -126,9 +126,8 @@ ParameterValue::ParameterValue(const rcl_interfaces::msg::ParameterValue & value
     case PARAMETER_INTEGER_ARRAY:
     case PARAMETER_DOUBLE_ARRAY:
     case PARAMETER_STRING_ARRAY:
-      break;
     case PARAMETER_NOT_SET:
-      throw std::runtime_error("Type from ParameterValue is not set");
+      break;
     default:
       // TODO(wjwwood): use custom exception
       throw std::runtime_error("Unknown type: " + std::to_string(value.type));
diff --git a/rclcpp/test/test_parameter.cpp b/rclcpp/test/test_parameter.cpp
index 6f7a120..01cbc00 100644
--- a/rclcpp/test/test_parameter.cpp
+++ b/rclcpp/test/test_parameter.cpp
@@ -53,8 +53,8 @@ TEST(TestParameter, not_set_variant) {
   EXPECT_EQ(rcl_interfaces::msg::ParameterType::PARAMETER_NOT_SET, not_set_param.value.type);
 
   // From parameter message
-  EXPECT_THROW(rclcpp::Parameter::from_parameter_msg(not_set_param),
-    std::runtime_error);
+  EXPECT_EQ(rclcpp::ParameterType::PARAMETER_NOT_SET,
+    rclcpp::Parameter::from_parameter_msg(not_set_param).get_type());
 }
 
 TEST(TestParameter, bool_variant) {