Make Node move constructor and assignment operator noexcept (#809)

Move constructor:
 * m_isValid    (bool)                 exchange(rhs.m_isValid, true)
 * m_invalidKey (std::string)          std::move()
 * m_pMemory    (shared_memory_holder) std::move()
 * m_pNode      (node*)                exchange(rhs.m_pNode, nullptr)

 This leaves the moved-from Node as if it was just default constructed.

Move assignment:
 A sanity test is performed to check if it's a valid move, and
 if not: *this is returned (with an added assert() for debugging).

 A temporary Node is move constructed (using the move constructor), leaving
 the moved-from Node as if it was just default constructed.

 If this->m_pNode == nullptr, the same operation as AssignNode would do is done
 and *this is returned.

 if temporary.m_pNode == nullptr:
  m_pNode->set_null()
  swap(*this, temporary)
  return *this;

 Otherwise the merge step that AssignNode would do if both m_pNodes are not
 nullptr is done, using a new member function, AssignNodeDetail().

Signed-off-by: Ted Lyngmo <ted@lyncon.se>

Author: TedLyngmo

include/yaml-cpp/node/impl.h

@@ -12,11 +12,29 @@
 #include "yaml-cpp/node/detail/node.h"
 #include "yaml-cpp/node/iterator.h"
 #include "yaml-cpp/node/node.h"
+#include "yaml-cpp/noexcept.h"
+#include <cassert>
 #include <sstream>
 #include <string>
+#include <utility>
 
 namespace YAML {
-inline Node::Node()
+namespace detail {
+#if __cplusplus >= 201402L
+using ::std::exchange;
+#else
+template<class T, class U = T>
+T exchange(T& obj, U&& new_value) {
+  T old_value = std::move(obj);
+  obj = std::forward<U>(new_value);
+  return old_value;
+}
+#endif
+}  // namespace detail
+}  // namespace YAML
+
+namespace YAML {
+inline Node::Node() YAML_CPP_NOEXCEPT
     : m_isValid(true), m_invalidKey{}, m_pMemory(nullptr), m_pNode(nullptr) {}
 
 inline Node::Node(NodeType::value type)
@@ -44,6 +62,13 @@ inline Node::Node(const detail::iterator_value& rhs)
 
 inline Node::Node(const Node&) = default;
 
+inline Node::Node(Node&& rhs) YAML_CPP_NOEXCEPT
+    : m_isValid(detail::exchange(rhs.m_isValid, true)),
+      m_invalidKey(std::move(rhs.m_invalidKey)),
+      m_pMemory(std::move(rhs.m_pMemory)),
+      m_pNode(detail::exchange(rhs.m_pNode, nullptr)) {
+}
+
 inline Node::Node(Zombie)
     : m_isValid(false), m_invalidKey{}, m_pMemory{}, m_pNode(nullptr) {}
 
@@ -194,6 +219,13 @@ inline void Node::SetStyle(EmitterStyle::value style) {
 }
 
 // assignment
+inline bool Node::CheckValidMove(const Node& rhs) const YAML_CPP_NOEXCEPT {
+  // Note: Both m_pNode's can be nullptr.
+  return
+    m_isValid && rhs.m_isValid &&
+    (!m_pNode || !rhs.m_pNode || !m_pNode->is(*rhs.m_pNode));
+}
+
 inline bool Node::is(const Node& rhs) const {
   if (!m_isValid || !rhs.m_isValid)
     throw InvalidNode(m_invalidKey);
@@ -215,6 +247,38 @@ inline Node& Node::operator=(const Node& rhs) {
   return *this;
 }
 
+inline Node& Node::operator=(Node&& rhs) YAML_CPP_NOEXCEPT {
+  if (!CheckValidMove(rhs)) {
+    assert(false);
+    return *this;
+  }
+
+  Node tmp(std::move(rhs));
+
+  if (!m_pNode) {
+    // we don't have a node so do what AssignNode does in this case
+    m_pNode = tmp.m_pNode;
+    m_pMemory = tmp.m_pMemory;
+    return *this;
+  }
+
+  if (!tmp.m_pNode) {
+    // We have an m_pNode but tmp doesn't. Instead of calling tmp.EnsureNodeExists(),
+    // which can potentially throw, and then AssignNodeDetail, we do set_null() on it
+    // and swap with the empty, but valid, tmp. While this does not ensure m_pNode
+    // to exist afterwards as before (quite the opposite), it leaves us in a valid
+    // state.
+    m_pNode->set_null();
+    std::swap(*this, tmp);
+    return *this;
+  }
+
+  // Both m_pNode's are present and the merge should (hopefully) be noexcept.
+  AssignNodeDetail(tmp);
+
+  return *this;
+}
+
 inline void Node::reset(const YAML::Node& rhs) {
   if (!m_isValid || !rhs.m_isValid)
     throw InvalidNode(m_invalidKey);
@@ -264,6 +328,10 @@ inline void Node::AssignNode(const Node& rhs) {
     return;
   }
 
+  AssignNodeDetail(rhs);
+}
+
+inline void Node::AssignNodeDetail(const Node& rhs) YAML_CPP_NOEXCEPT {
   m_pNode->set_ref(*rhs.m_pNode);
   m_pMemory->merge(*rhs.m_pMemory);
   m_pNode = rhs.m_pNode;

include/yaml-cpp/node/node.h

@@ -16,6 +16,7 @@
 #include "yaml-cpp/node/detail/iterator_fwd.h"
 #include "yaml-cpp/node/ptr.h"
 #include "yaml-cpp/node/type.h"
+#include "yaml-cpp/noexcept.h"
 
 namespace YAML {
 namespace detail {
@@ -41,12 +42,13 @@ class YAML_CPP_API Node {
   using iterator = YAML::iterator;
   using const_iterator = YAML::const_iterator;
 
-  Node();
+  Node() YAML_CPP_NOEXCEPT;
   explicit Node(NodeType::value type);
   template <typename T>
   explicit Node(const T& rhs);
   explicit Node(const detail::iterator_value& rhs);
   Node(const Node& rhs);
+  Node(Node&& rhs) YAML_CPP_NOEXCEPT;
   ~Node();
 
   YAML::Mark Mark() const;
@@ -77,10 +79,12 @@ class YAML_CPP_API Node {
   void SetStyle(EmitterStyle::value style);
 
   // assignment
+  bool CheckValidMove(const Node& rhs) const YAML_CPP_NOEXCEPT;
   bool is(const Node& rhs) const;
   template <typename T>
   Node& operator=(const T& rhs);
   Node& operator=(const Node& rhs);
+  Node& operator=(Node&& rhs) YAML_CPP_NOEXCEPT;
   void reset(const Node& rhs = Node());
 
   // size/iterator
@@ -128,6 +132,7 @@ class YAML_CPP_API Node {
 
   void AssignData(const Node& rhs);
   void AssignNode(const Node& rhs);
+  void AssignNodeDetail(const Node& rhs) YAML_CPP_NOEXCEPT;
 
  private:
   bool m_isValid;

test/node/node_test.cpp

@@ -184,6 +184,22 @@ TEST(NodeTest, EqualRepresentationAfterMoveAssignment) {
   EXPECT_EQ(ss1.str(), ss2.str());
 }
 
+TEST(NodeTest, NodeIsNullWhenMovedFromByCtor) {
+  Node node1;
+  node1[1] = 1;
+  Node node2 = std::move(node1);
+  EXPECT_TRUE(node1.IsNull());
+}
+
+TEST(NodeTest, NodeIsNullWhenMovedFromByAssignment) {
+  Node node1;
+  Node node2;
+  node1[1] = 1;
+  node2[2] = 2;
+  node2 = std::move(node1);
+  EXPECT_TRUE(node1.IsNull());
+}
+
 TEST(NodeTest, MapElementRemoval) {
   Node node;
   node["foo"] = "bar";