Merge pull request #10353 from Icinga/fix-string-move-constructor-and-assignment-operator

Fix string move constructor and assignment operator

lib/base/string.cpp

@@ -33,7 +33,7 @@ String::String(const String& other)
 	: m_Data(other)
 { }
 
-String::String(String&& other)
+String::String(String&& other) noexcept
 	: m_Data(std::move(other.m_Data))
 { }
 
@@ -47,7 +47,7 @@ String::String(Value&& other)
 String& String::operator=(Value&& other)
 {
 	if (other.IsString())
-		m_Data = std::move(other.Get<String>());
+		*this = std::move(other.Get<String>()); // Will atomically bind to the move assignment operator below.
 	else
 		*this = static_cast<String>(other);
 
@@ -66,7 +66,7 @@ String& String::operator=(const String& rhs)
 	return *this;
 }
 
-String& String::operator=(String&& rhs)
+String& String::operator=(String&& rhs) noexcept
 {
 	m_Data = std::move(rhs.m_Data);
 	return *this;

lib/base/string.hpp

@@ -44,7 +44,7 @@ public:
 	String(std::string data);
 	String(String::SizeType n, char c);
 	String(const String& other);
-	String(String&& other);
+	String(String&& other) noexcept;
 
 #ifndef _MSC_VER
 	String(Value&& other);
@@ -56,7 +56,7 @@ public:
 	{ }
 
 	String& operator=(const String& rhs);
-	String& operator=(String&& rhs);
+	String& operator=(String&& rhs) noexcept;
 	String& operator=(Value&& rhs);
 	String& operator=(const std::string& rhs);
 	String& operator=(const char *rhs);

lib/base/value.cpp

@@ -9,9 +9,13 @@ using namespace icinga;
 
 template class boost::variant<boost::blank, double, bool, String, Object::Ptr>;
 template const double& Value::Get<double>() const;
+template double& Value::Get<double>();
 template const bool& Value::Get<bool>() const;
+template bool& Value::Get<bool>();
 template const String& Value::Get<String>() const;
+template String& Value::Get<String>();
 template const Object::Ptr& Value::Get<Object::Ptr>() const;
+template Object::Ptr& Value::Get<Object::Ptr>();
 
 const Value icinga::Empty;
 

lib/base/value.hpp

@@ -140,14 +140,24 @@ public:
 		return boost::get<T>(m_Value);
 	}
 
+	template<typename T>
+	T& Get()
+	{
+		return boost::get<T>(m_Value);
+	}
+
 private:
 	boost::variant<boost::blank, double, bool, String, Object::Ptr> m_Value;
 };
 
 extern template const double& Value::Get<double>() const;
+extern template double& Value::Get<double>();
 extern template const bool& Value::Get<bool>() const;
+extern template bool& Value::Get<bool>();
 extern template const String& Value::Get<String>() const;
+extern template String& Value::Get<String>();
 extern template const Object::Ptr& Value::Get<Object::Ptr>() const;
+extern template Object::Ptr& Value::Get<Object::Ptr>();
 
 extern const Value Empty;
 

test/CMakeLists.txt

@@ -165,6 +165,8 @@ add_boost_test(base
     base_string/replace
     base_string/index
     base_string/find
+    base_string/vector_move
+    base_string/move_string_out_of_Value_type
     base_timer/construct
     base_timer/interval
     base_timer/invoke

test/base-string.cpp

@@ -1,6 +1,8 @@
 /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */
 
 #include "base/string.hpp"
+#include "base/value.hpp"
+#include <vector>
 #include <BoostTestTargetConfig.h>
 
 using namespace icinga;
@@ -101,4 +103,44 @@ BOOST_AUTO_TEST_CASE(find)
 	BOOST_CHECK(s.FindFirstOf("xl") == 2);
 }
 
+// Check that if a std::vector<icinga::String> is grown beyond its capacity (i.e. it has to reallocate the memory),
+// it uses the move constructor of icinga::String (i.e. the underlying string storage stays the same).
+BOOST_AUTO_TEST_CASE(vector_move)
+{
+	std::vector<String> vec {
+		// std::string (which is internally used by icinga::String) has an optimization that small strings can be
+		// allocated inside it instead of in a separate heap allocation. In that case, the small string would still be
+		// copied even by the move constructor. Using sizeof() ensures that the string is long enough so that it must
+		// be allocated separately and can be used to test for the desired move to happen.
+		std::string(sizeof(String) + 1, 'A'),
+	};
+
+	void *oldAddr = vec[0].GetData().data();
+	// Sanity check that the data buffer is actually allocated outside the icinga::String instance.
+	BOOST_CHECK(!(&vec[0] <= oldAddr && oldAddr < &vec[1]));
+
+	// Force the vector to grow.
+	vec.reserve(vec.capacity() + 1);
+
+	// If the string was moved, the location of its underlying data buffer should not have changed.
+	void *newAddr = vec[0].GetData().data();
+	BOOST_CHECK_EQUAL(oldAddr, newAddr);
+}
+
+// Test that the move constructor of icinga::String actually moves the underlying std::string out of a Value instance.
+// The constructor overload is only available on non-Windows platforms though, so we need to skip the test on Windows.
+BOOST_AUTO_TEST_CASE(move_string_out_of_Value_type)
+{
+#ifndef _MSC_VER
+	Value value("Icinga 2");
+	String other = value.Get<String>(); // We didn't request a move, so this should just copy the string.
+	BOOST_CHECK_EQUAL("Icinga 2", value.Get<String>());
+	BOOST_CHECK_EQUAL("Icinga 2", other);
+
+	String newStr = std::move(value);
+	BOOST_CHECK_EQUAL("", value.Get<String>());
+	BOOST_CHECK_EQUAL(newStr, "Icinga 2");
+#endif /* _MSC_VER */
+}
+
 BOOST_AUTO_TEST_SUITE_END()