Comment move behavior for fxcrt string and pointer types.
Then add tests to ensure it is always so.
- Tidy some friending while at it.
Bug: chromium:1039257
Change-Id: I579828a6d269801122645bc345136530a4b3fe96
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/64650
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fxcrt/bytestring.h b/core/fxcrt/bytestring.h
index f939213..11c372b 100644
--- a/core/fxcrt/bytestring.h
+++ b/core/fxcrt/bytestring.h
@@ -23,10 +23,6 @@
namespace fxcrt {
-class ByteString_Assign_Test;
-class ByteString_Concat_Test;
-class StringPool_ByteString_Test;
-
// A mutable string with shared buffers using copy-on-write semantics that
// avoids the cost of std::string's iterator stability guarantees.
class ByteString {
@@ -43,6 +39,8 @@
ByteString();
ByteString(const ByteString& other);
+
+ // Move-construct a ByteString. After construction, |other| is empty.
ByteString(ByteString&& other) noexcept;
// Deliberately implicit to avoid calling on every string literal.
@@ -134,6 +132,8 @@
ByteString& operator=(const char* str);
ByteString& operator=(ByteStringView str);
ByteString& operator=(const ByteString& that);
+
+ // Move-assign a ByteString. After assignment, |that| is empty.
ByteString& operator=(ByteString&& that);
ByteString& operator+=(char ch);
@@ -211,8 +211,9 @@
RetainPtr<StringData> m_pData;
- friend ByteString_Assign_Test;
- friend ByteString_Concat_Test;
+ friend class ByteString_Assign_Test;
+ friend class ByteString_Concat_Test;
+ friend class ByteString_Construct_Test;
friend class StringPool_ByteString_Test;
};
diff --git a/core/fxcrt/bytestring_unittest.cpp b/core/fxcrt/bytestring_unittest.cpp
index f40784b..d0eab1a 100644
--- a/core/fxcrt/bytestring_unittest.cpp
+++ b/core/fxcrt/bytestring_unittest.cpp
@@ -60,6 +60,27 @@
#endif
}
+TEST(ByteString, Construct) {
+ {
+ // Copy-construct.
+ ByteString string1("abc");
+ ByteString string2(string1);
+ EXPECT_EQ("abc", string1);
+ EXPECT_EQ("abc", string2);
+ EXPECT_EQ(2, string1.ReferenceCountForTesting());
+ EXPECT_EQ(2, string2.ReferenceCountForTesting());
+ }
+ {
+ // Move-construct.
+ ByteString string1("abc");
+ ByteString string2(std::move(string1));
+ EXPECT_TRUE(string1.IsEmpty());
+ EXPECT_EQ("abc", string2);
+ EXPECT_EQ(0, string1.ReferenceCountForTesting());
+ EXPECT_EQ(1, string2.ReferenceCountForTesting());
+ }
+}
+
TEST(ByteString, Assign) {
{
// Copy-assign.
@@ -84,6 +105,8 @@
EXPECT_EQ(1, string2.ReferenceCountForTesting());
string1 = std::move(string2);
+ EXPECT_EQ("abc", string1);
+ EXPECT_TRUE(string2.IsEmpty());
EXPECT_EQ(1, string1.ReferenceCountForTesting());
EXPECT_EQ(0, string2.ReferenceCountForTesting());
}
diff --git a/core/fxcrt/retain_ptr.h b/core/fxcrt/retain_ptr.h
index 4507d69..916a12e 100644
--- a/core/fxcrt/retain_ptr.h
+++ b/core/fxcrt/retain_ptr.h
@@ -31,6 +31,8 @@
RetainPtr() = default;
RetainPtr(const RetainPtr& that) : RetainPtr(that.Get()) {}
+
+ // Move-construct a RetainPtr. After construction, |that| will be NULL.
RetainPtr(RetainPtr&& that) noexcept { Swap(that); }
// Deliberately implicit to allow returning nullptrs.
@@ -65,6 +67,7 @@
return *this;
}
+ // Move-assign a RetainPtr. After assignment, |that| will be NULL.
RetainPtr& operator=(RetainPtr&& that) {
m_pObj.reset(that.Leak());
return *this;
diff --git a/core/fxcrt/unowned_ptr.h b/core/fxcrt/unowned_ptr.h
index 5a8ee3d..f7ff480 100644
--- a/core/fxcrt/unowned_ptr.h
+++ b/core/fxcrt/unowned_ptr.h
@@ -49,6 +49,8 @@
public:
constexpr UnownedPtr() noexcept = default;
constexpr UnownedPtr(const UnownedPtr& that) noexcept = default;
+
+ // Move-construct an UnownedPtr. After construction, |that| will be NULL.
constexpr UnownedPtr(UnownedPtr&& that) noexcept : m_pObj(that.Release()) {}
template <typename U>
@@ -76,6 +78,7 @@
return *this;
}
+ // Move-assign an UnownedPtr. After assignment, |that| will be NULL.
UnownedPtr& operator=(UnownedPtr&& that) noexcept {
if (*this != that)
Reset(that.Release());
diff --git a/core/fxcrt/unowned_ptr_unittest.cpp b/core/fxcrt/unowned_ptr_unittest.cpp
index f1979ef..fa884d4 100644
--- a/core/fxcrt/unowned_ptr_unittest.cpp
+++ b/core/fxcrt/unowned_ptr_unittest.cpp
@@ -120,6 +120,7 @@
auto owned = pdfium::MakeUnique<Clink>();
outer = owned.get();
UnownedPtr<Clink> inner(std::move(outer));
+ EXPECT_EQ(nullptr, outer.Get());
}
}
@@ -130,6 +131,7 @@
outer = owned.get();
UnownedPtr<Clink> inner;
inner = std::move(outer);
+ EXPECT_EQ(nullptr, outer.Get());
}
}
diff --git a/core/fxcrt/widestring.h b/core/fxcrt/widestring.h
index 3373067..a9b10ce 100644
--- a/core/fxcrt/widestring.h
+++ b/core/fxcrt/widestring.h
@@ -23,9 +23,6 @@
namespace fxcrt {
class ByteString;
-class StringPool_WideString_Test;
-class WideString_Assign_Test;
-class WideString_ConcatInPlace_Test;
// A mutable string with shared buffers using copy-on-write semantics that
// avoids the cost of std::string's iterator stability guarantees.
@@ -41,6 +38,8 @@
WideString();
WideString(const WideString& other);
+
+ // Move-construct a WideString. After construction, |other| is empty.
WideString(WideString&& other) noexcept;
// Deliberately implicit to avoid calling on every string literal.
@@ -116,6 +115,8 @@
WideString& operator=(const wchar_t* str);
WideString& operator=(WideStringView str);
WideString& operator=(const WideString& that);
+
+ // Move-assign a WideString. After assignment, |that| is empty.
WideString& operator=(WideString&& that);
WideString& operator+=(const wchar_t* str);
@@ -227,9 +228,10 @@
RetainPtr<StringData> m_pData;
- friend WideString_ConcatInPlace_Test;
- friend WideString_Assign_Test;
- friend StringPool_WideString_Test;
+ friend class WideString_Assign_Test;
+ friend class WideString_ConcatInPlace_Test;
+ friend class WideString_Construct_Test;
+ friend class StringPool_WideString_Test;
};
inline WideString operator+(WideStringView str1, WideStringView str2) {
diff --git a/core/fxcrt/widestring_unittest.cpp b/core/fxcrt/widestring_unittest.cpp
index f64aada..8d25476 100644
--- a/core/fxcrt/widestring_unittest.cpp
+++ b/core/fxcrt/widestring_unittest.cpp
@@ -57,6 +57,27 @@
#endif
}
+TEST(WideString, Construct) {
+ {
+ // Copy-construct.
+ WideString string1(L"abc");
+ WideString string2(string1);
+ EXPECT_EQ(L"abc", string1);
+ EXPECT_EQ(L"abc", string2);
+ EXPECT_EQ(2, string1.ReferenceCountForTesting());
+ EXPECT_EQ(2, string2.ReferenceCountForTesting());
+ }
+ {
+ // Move-construct.
+ WideString string1(L"abc");
+ WideString string2(std::move(string1));
+ EXPECT_TRUE(string1.IsEmpty());
+ EXPECT_EQ(L"abc", string2);
+ EXPECT_EQ(0, string1.ReferenceCountForTesting());
+ EXPECT_EQ(1, string2.ReferenceCountForTesting());
+ }
+}
+
TEST(WideString, Assign) {
{
// Copy-assign.
@@ -81,6 +102,8 @@
EXPECT_EQ(1, string2.ReferenceCountForTesting());
string1 = std::move(string2);
+ EXPECT_EQ(L"abc", string1);
+ EXPECT_TRUE(string2.IsEmpty());
EXPECT_EQ(1, string1.ReferenceCountForTesting());
EXPECT_EQ(0, string2.ReferenceCountForTesting());
}
