Utils: SmallString trades copies for branches

There can be quite some copies with the swap method for the default case
that the string is inside the small string area. Instead now we minimize
copies of the smallstring area at the expense of more branches.

The swap approch needs no self tests and it uses the destructor to
remove allocated resources but it can copy three times more often. And
the destructor still has an additional branch in the copy case. Only in
the move case we can use the destructor of the other instance. This can
be surprising for some users which expect that the instance is empty
after a move(actually assuming that a instance is empty after a move is
undefined behavior).

Because SmallString is designed to hold the string in almost all cases
inside the small string area that case should be optimized.

Change-Id: I22cb62aa99b9713a221f103971f149c05e9ff6fa
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

src/libs/utils/smallstring.h

@@ -170,19 +170,25 @@ public:
             Memory::deallocate(m_data.allocated.data.pointer);
     }
 
-    BasicSmallString(const BasicSmallString &string)
+    BasicSmallString(const BasicSmallString &other)
     {
-        if (string.isShortString() || string.isReadOnlyReference())
+        if (Q_LIKELY(other.isShortString() || other.isReadOnlyReference()))
-            m_data = string.m_data;
+            m_data = other.m_data;
         else
-            new (this) BasicSmallString{string.data(), string.size()};
+            new (this) BasicSmallString{other.data(), other.size()};
     }
 
     BasicSmallString &operator=(const BasicSmallString &other)
     {
-        BasicSmallString copy = other;
+        if (Q_LIKELY(this != &other)) {
+            if (Q_UNLIKELY(hasAllocatedMemory()))
+                Memory::deallocate(m_data.allocated.data.pointer);
 
-        swap(*this, copy);
+            if (Q_LIKELY(other.isShortString() || other.isReadOnlyReference()))
+                m_data = other.m_data;
+            else
+                new (this) BasicSmallString{other.data(), other.size()};
+        }
 
         return *this;
     }
@@ -195,17 +201,18 @@ public:
 
     BasicSmallString &operator=(BasicSmallString &&other) noexcept
     {
-        swap(*this, other);
+        if (Q_LIKELY(this != &other)) {
+            if (Q_UNLIKELY(hasAllocatedMemory()))
+                Memory::deallocate(m_data.allocated.data.pointer);
+
+            m_data = std::move(other.m_data);
+            other.m_data.reset();
+        }
 
         return *this;
     }
 
-    BasicSmallString take() noexcept
+    BasicSmallString take() noexcept { return std::move(*this); }
-    {
-        auto tmp = std::move(*this);
-
-        return tmp;
-    }
     BasicSmallString clone() const
     {
         BasicSmallString clonedString(m_data);

tests/unit/unittest/smallstring-test.cpp

@@ -1518,7 +1518,6 @@ TEST(SmallString, ShortSmallStringMoveConstuctor)
 
     auto copy = std::move(text);
 
-    ASSERT_TRUE(text.isEmpty());
     ASSERT_THAT(copy, SmallString("text"));
 }
 
@@ -1528,7 +1527,6 @@ TEST(SmallString, LongSmallStringMoveConstuctor)
 
     auto copy = std::move(text);
 
-    ASSERT_TRUE(text.isEmpty());
     ASSERT_THAT(copy, SmallString("this is a very very very very long text"));
 }
 
@@ -1538,7 +1536,6 @@ TEST(SmallString, ShortPathStringMoveConstuctor)
 
     auto copy = std::move(text);
 
-    ASSERT_TRUE(text.isEmpty());
     ASSERT_THAT(copy, SmallString("text"));
 }
 
@@ -1551,7 +1548,6 @@ TEST(SmallString, LongPathStringMoveConstuctor)
 
     auto copy = std::move(text);
 
-    ASSERT_TRUE(text.isEmpty());
     ASSERT_THAT(
         copy,
         PathString(
@@ -1642,7 +1638,6 @@ TEST(SmallString, ShortSmallStringMoveAssignment)
 
     copy = std::move(text);
 
-    ASSERT_THAT(text, SmallString("more text"));
     ASSERT_THAT(copy, SmallString("text"));
 }
 
@@ -1653,10 +1648,38 @@ TEST(SmallString, LongSmallStringMoveAssignment)
 
     copy = std::move(text);
 
-    ASSERT_THAT(text, SmallString("more text"));
     ASSERT_THAT(copy, SmallString("this is a very very very very long text"));
 }
 
+TEST(SmallString, ShortPathStringMoveAssignment)
+{
+    PathString text("text");
+    PathString copy("more text");
+
+    copy = std::move(text);
+
+    ASSERT_THAT(copy, SmallString("text"));
+}
+
+TEST(SmallString, LongPathStringMoveAssignment)
+{
+    PathString text(
+        "this is a very very very very very very very very very very very very very very very very "
+        "very very very very very very very very very very very very very very very very very very "
+        "very very very very very very very very very very very very very very long text");
+    PathString copy("more text");
+
+    copy = std::move(text);
+
+    ASSERT_THAT(
+        copy,
+        PathString(
+            "this is a very very very very very very very very very very very very very very very "
+            "very very very very very very very very very very very very very very very very very "
+            "very very very very very very very very very very very very very very very very long "
+            "text"));
+}
+
 TEST(SmallString, ShortSmallStringTake)
 {
     SmallString text("text");