From 57e1b3634dccf49c37f2cd9c9eaa74dd682e617a Mon Sep 17 00:00:00 2001 From: Ted Lyngmo Date: Wed, 22 Jan 2020 11:40:50 +0100 Subject: [PATCH] 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 --- include/yaml-cpp/node/impl.h | 70 +++++++++++++++++++++++++++++++++++- include/yaml-cpp/node/node.h | 7 +++- test/node/node_test.cpp | 16 +++++++++ 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/include/yaml-cpp/node/impl.h b/include/yaml-cpp/node/impl.h index f5d622b25..ad8d694dd 100644 --- a/include/yaml-cpp/node/impl.h +++ b/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 #include #include +#include namespace YAML { -inline Node::Node() +namespace detail { +#if __cplusplus >= 201402L +using ::std::exchange; +#else +template +T exchange(T& obj, U&& new_value) { + T old_value = std::move(obj); + obj = std::forward(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; diff --git a/include/yaml-cpp/node/node.h b/include/yaml-cpp/node/node.h index c9e9a0a4b..07af4d928 100644 --- a/include/yaml-cpp/node/node.h +++ b/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 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 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; diff --git a/test/node/node_test.cpp b/test/node/node_test.cpp index f889059a7..ad68cdeef 100644 --- a/test/node/node_test.cpp +++ b/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";