From 1a06016154a0dfdffbf1cd8b07905d11ff5532bd Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 2 Oct 2022 18:20:49 +0200 Subject: [PATCH] Implement move semantics for xml::attributes This allows to move them cheaply (or at least cheaper than by copying everything). Switch to representing null pimpl_ pointer for representing the "empty" state, as is more compatible with move semantics, but this required adding checks for the pointer being null in a number of places. --- include/xmlwrapp/attributes.h | 28 ++++++++++++++++ src/libxml/ait_impl.cxx | 49 +++++++++++++++++++++------- src/libxml/ait_impl.h | 4 +-- src/libxml/attributes.cxx | 28 +++++++++++----- tests/attributes/test_attributes.cxx | 33 +++++++++++++++++++ 5 files changed, 119 insertions(+), 23 deletions(-) diff --git a/include/xmlwrapp/attributes.h b/include/xmlwrapp/attributes.h index 2f18b795..26e72aa6 100644 --- a/include/xmlwrapp/attributes.h +++ b/include/xmlwrapp/attributes.h @@ -70,6 +70,11 @@ struct node_impl; The iterator classes allow you to access one XML attribute. This is done using the xml::attributes::attr class interface. + + Objects of this class are copyable and movable, with the latter being more + efficient, but leaving the source object in a special state in which only + asignment operator and empty() and size() member functions can be used on + it. */ class XMLWRAPP_API attributes { @@ -89,6 +94,15 @@ class XMLWRAPP_API attributes */ attributes(const attributes& other); + /** + Move construct a xml::attributes object. + + @param other The xml::attributes object to move from. + + @since 0.10.0 + */ + attributes(attributes&& other); + /** Copy the given xml::attributes object into this one. @@ -97,6 +111,16 @@ class XMLWRAPP_API attributes */ attributes& operator=(const attributes& other); + /** + Move the given xml::attributes object into this one. + + @param other The xml::attributes object to move from. + @return *this. + + @since 0.10.0 + */ + attributes& operator=(attributes&& other); + /** Swap this xml::attributes object with another one. @@ -162,7 +186,9 @@ class XMLWRAPP_API attributes iterator(); iterator(const iterator& other); + iterator(iterator&& other); iterator& operator=(const iterator& other); + iterator& operator=(iterator&& other); ~iterator(); reference operator*() const; @@ -203,8 +229,10 @@ class XMLWRAPP_API attributes const_iterator(); const_iterator(const const_iterator& other); + const_iterator(const_iterator&& other); const_iterator(const iterator& other); const_iterator& operator=(const const_iterator& other); + const_iterator& operator=(const_iterator&& other); ~const_iterator(); reference operator*() const; diff --git a/src/libxml/ait_impl.cxx b/src/libxml/ait_impl.cxx index 49814ed2..d73a5188 100644 --- a/src/libxml/ait_impl.cxx +++ b/src/libxml/ait_impl.cxx @@ -132,7 +132,6 @@ ait_impl ait_impl::operator++(int) // ------------------------------------------------------------------------ attributes::iterator::iterator() - : pimpl_{new ait_impl(nullptr, nullptr)} { } @@ -163,6 +162,10 @@ attributes::iterator& attributes::iterator::operator=(const iterator& other) } +attributes::iterator::iterator (iterator&&) = default; +attributes::iterator& attributes::iterator::operator=(iterator&&) = default; + + void attributes::iterator::swap(iterator& other) { std::swap(pimpl_, other.pimpl_); @@ -210,7 +213,6 @@ attributes::iterator attributes::iterator::operator++(int) // ------------------------------------------------------------------------ attributes::const_iterator::const_iterator() - : pimpl_{new ait_impl(nullptr, nullptr)} { } @@ -247,6 +249,10 @@ attributes::const_iterator& attributes::const_iterator::operator=(const const_it } +attributes::const_iterator::const_iterator(const_iterator&&) = default; +attributes::const_iterator& attributes::const_iterator::operator=(const_iterator&&) = default; + + void attributes::const_iterator::swap(const_iterator& other) { std::swap(pimpl_, other.pimpl_); @@ -355,7 +361,7 @@ const char* attributes::attr::get_value() const bool operator==(const attributes::iterator& lhs, const attributes::iterator& rhs) { - return *(lhs.pimpl_) == *(rhs.pimpl_); + return ait_impl::are_equal(lhs.pimpl_.get(), rhs.pimpl_.get()); } bool operator!=(const attributes::iterator& lhs, const attributes::iterator& rhs) @@ -365,7 +371,7 @@ bool operator!=(const attributes::iterator& lhs, const attributes::iterator& rhs bool operator==(const attributes::const_iterator& lhs, const attributes::const_iterator& rhs) { - return *(lhs.pimpl_) == *(rhs.pimpl_); + return ait_impl::are_equal(lhs.pimpl_.get(), rhs.pimpl_.get()); } bool operator!=(const attributes::const_iterator& lhs, const attributes::const_iterator& rhs) @@ -414,16 +420,35 @@ xmlAttributePtr find_default_prop(xmlNodePtr xmlnode, const char *name) return nullptr; } -bool operator==(const ait_impl& lhs, const ait_impl& rhs) +bool ait_impl::are_equal(const ait_impl* lhs, const ait_impl* rhs) { - if (lhs.fake_ || rhs.fake_) - return false; - return lhs.xmlattr_ == rhs.xmlattr_; -} + xmlAttrPtr lhsAttr; + if (lhs) + { + if (lhs->fake_) + return false; -bool operator!=(const ait_impl& lhs, const ait_impl& rhs) -{ - return !(lhs == rhs); + lhsAttr = lhs->xmlattr_; + } + else + { + lhsAttr = nullptr; + } + + xmlAttrPtr rhsAttr; + if (rhs) + { + if (rhs->fake_) + return false; + + rhsAttr = rhs->xmlattr_; + } + else + { + rhsAttr = nullptr; + } + + return lhsAttr == rhsAttr; } } // namespace impl diff --git a/src/libxml/ait_impl.h b/src/libxml/ait_impl.h index 3975114f..901ad2a2 100644 --- a/src/libxml/ait_impl.h +++ b/src/libxml/ait_impl.h @@ -61,8 +61,8 @@ class ait_impl ait_impl& operator++(); ait_impl operator++(int); - friend bool operator==(const ait_impl& lhs, const ait_impl& rhs); - friend bool operator!=(const ait_impl& lhs, const ait_impl& rhs); + // Pointer arguments may be null, representing invalid attribute. + static bool are_equal(const ait_impl* lhs, const ait_impl* rhs); private: xmlNodePtr xmlnode_; diff --git a/src/libxml/attributes.cxx b/src/libxml/attributes.cxx index d694da9c..966babcc 100644 --- a/src/libxml/attributes.cxx +++ b/src/libxml/attributes.cxx @@ -112,24 +112,27 @@ attributes::attributes() attributes::attributes(int) - : pimpl_{new pimpl(nullptr)} { } attributes::attributes(const attributes& other) - : pimpl_{new pimpl(*other.pimpl_)} { + if (other.pimpl_) + pimpl_.reset(new pimpl(*other.pimpl_)); } attributes& attributes::operator=(const attributes& other) { - attributes tmp(other); - swap(tmp); + pimpl_.reset(other.pimpl_ ? new pimpl(*other.pimpl_) : nullptr); return *this; } +attributes::attributes(attributes&&) = default; + +attributes& attributes::operator=(attributes&&) = default; + void attributes::swap(attributes& other) { @@ -150,9 +153,16 @@ void attributes::set_data(void *node) { auto x = static_cast(node); - pimpl_->free_node_if_owned(); - pimpl_->owner_ = false; - pimpl_->xmlnode_ = x; + if (pimpl_) + { + pimpl_->free_node_if_owned(); + pimpl_->owner_ = false; + pimpl_->xmlnode_ = x; + } + else + { + pimpl_.reset(new pimpl(x)); + } } @@ -241,7 +251,7 @@ void attributes::erase(const char *name) bool attributes::empty() const { - return pimpl_->xmlnode_->properties == nullptr; + return !pimpl_ || pimpl_->xmlnode_->properties == nullptr; } @@ -249,7 +259,7 @@ attributes::size_type attributes::size() const { size_type count = 0; - xmlAttrPtr prop = pimpl_->xmlnode_->properties; + xmlAttrPtr prop = pimpl_ ? pimpl_->xmlnode_->properties : nullptr; while (prop != nullptr) { ++count; diff --git a/tests/attributes/test_attributes.cxx b/tests/attributes/test_attributes.cxx index f2fe7bfb..e0974844 100644 --- a/tests/attributes/test_attributes.cxx +++ b/tests/attributes/test_attributes.cxx @@ -296,3 +296,36 @@ TEST_CASE_METHOD( SrcdirConfig, "attributes/compare_attr_iterators", "[attribute CHECK( ci == i ); CHECK( !(ci != i) ); } + +/* + * Test to see if moving attribute objects works. + */ +TEST_CASE_METHOD( SrcdirConfig, "attributes/move", "[attributes]" ) +{ + xml::tree_parser parser(test_file_path("attributes/data/06b.xml").c_str()); + + xml::attributes attrs1 = parser.get_document().get_root_node().get_attributes(); + CHECK( !attrs1.empty() ); + + xml::attributes attrs2{std::move(attrs1)}; + CHECK( attrs1.empty() ); + CHECK( !attrs2.empty() ); + + xml::attributes attrs3; + attrs3 = std::move(attrs2); + CHECK( attrs2.empty() ); + CHECK( !attrs3.empty() ); + + auto const end = xml::attributes::iterator{}; + auto i1 = attrs3.begin(); + CHECK( i1 != end ); + + auto i2{std::move(i1)}; + CHECK( i1 == end ); + CHECK( i2 != end ); + + decltype(i2) i3; + i3 = std::move(i2); + CHECK( i2 == end ); + CHECK( i3 != end ); +}