From e38d1f06bd81adc1b7c077ad164e24f722456c6f Mon Sep 17 00:00:00 2001
From: Eike Ziller <eike.ziller@qt.io>
Date: Thu, 22 Dec 2022 09:40:34 +0100
Subject: [PATCH] Fix that Utils::sorted could modify input container

Utils::sorted had overloads for "const Container &", "Container &&", and
"const Container &&", but for _templated_ types "Container &&" does
_not_ mean "rvalue reference", it means "rvalue or lvalue reference"
(e.g. "universal" reference).

That means that for non-const lvalue references that "Container &&"
overload was used, which modifies the input container. Which is a fine
optimization for rvalue references, but is wrong for lvalue references.

Add another overload explicitly for "Container &" before the
"Container &&" overload, and add some tests.

Also fix the compiler warning that triggered the investigation:

warning: local variable 'container' will be copied despite being
returned by name [-Wreturn-std-move]
note: call 'std::move' explicitly to avoid copying

Amends 13f40f5471e55757a2cf9bba8d052750a2f2a753

Change-Id: I14461fde5fc51a8bb679fd72b886e13b18c47e7b
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: hjk <hjk@qt.io>
---
 src/libs/utils/algorithm.h             |  92 +++++++++++++++++-----
 tests/auto/algorithm/tst_algorithm.cpp | 101 +++++++++++++++++++++++++
 2 files changed, 175 insertions(+), 18 deletions(-)

diff --git a/src/libs/utils/algorithm.h b/src/libs/utils/algorithm.h
index 71fb2ad35d0..0413c816fa4 100644
--- a/src/libs/utils/algorithm.h
+++ b/src/libs/utils/algorithm.h
@@ -1005,7 +1005,8 @@ inline void sort(Container &container, Predicate p)
     std::stable_sort(std::begin(container), std::end(container), p);
 }
 
-template <typename Container>
+// const lvalue
+template<typename Container>
 inline Container sorted(const Container &container)
 {
     Container c = container;
@@ -1013,20 +1014,34 @@ inline Container sorted(const Container &container)
     return c;
 }
 
-template <typename Container>
+// non-const lvalue
+// This is needed because otherwise the "universal" reference below is used, modifying the input
+// container.
+template<typename Container>
+inline Container sorted(Container &container)
+{
+    Container c = container;
+    sort(c);
+    return c;
+}
+
+// non-const rvalue (actually rvalue or lvalue, but lvalue is handled above)
+template<typename Container>
 inline Container sorted(Container &&container)
 {
     sort(container);
-    return container;
+    return std::move(container);
 }
 
-template <typename Container>
+// const rvalue
+template<typename Container>
 inline Container sorted(const Container &&container)
 {
     return sorted(container);
 }
 
-template <typename Container, typename Predicate>
+// const lvalue
+template<typename Container, typename Predicate>
 inline Container sorted(const Container &container, Predicate p)
 {
     Container c = container;
@@ -1034,21 +1049,34 @@ inline Container sorted(const Container &container, Predicate p)
     return c;
 }
 
-template <typename Container, typename Predicate>
+// non-const lvalue
+// This is needed because otherwise the "universal" reference below is used, modifying the input
+// container.
+template<typename Container, typename Predicate>
+inline Container sorted(Container &container, Predicate p)
+{
+    Container c = container;
+    sort(c, p);
+    return c;
+}
+
+// non-const rvalue (actually rvalue or lvalue, but lvalue is handled above)
+template<typename Container, typename Predicate>
 inline Container sorted(Container &&container, Predicate p)
 {
     sort(container, p);
-    return container;
+    return std::move(container);
 }
 
-template <typename Container, typename Predicate>
+// const rvalue
+template<typename Container, typename Predicate>
 inline Container sorted(const Container &&container, Predicate p)
 {
     return sorted(container, p);
 }
 
 // pointer to member
-template <typename Container, typename R, typename S>
+template<typename Container, typename R, typename S>
 inline void sort(Container &container, R S::*member)
 {
     auto f = std::mem_fn(member);
@@ -1059,7 +1087,8 @@ inline void sort(Container &container, R S::*member)
     });
 }
 
-template <typename Container, typename R, typename S>
+// const lvalue
+template<typename Container, typename R, typename S>
 inline Container sorted(const Container &container, R S::*member)
 {
     Container c = container;
@@ -1067,21 +1096,34 @@ inline Container sorted(const Container &container, R S::*member)
     return c;
 }
 
-template <typename Container, typename R, typename S>
+// non-const lvalue
+// This is needed because otherwise the "universal" reference below is used, modifying the input
+// container.
+template<typename Container, typename R, typename S>
+inline Container sorted(Container &container, R S::*member)
+{
+    Container c = container;
+    sort(c, member);
+    return c;
+}
+
+// non-const rvalue (actually rvalue or lvalue, but lvalue is handled above)
+template<typename Container, typename R, typename S>
 inline Container sorted(Container &&container, R S::*member)
 {
     sort(container, member);
-    return container;
+    return std::move(container);
 }
 
-template <typename Container, typename R, typename S>
+// const rvalue
+template<typename Container, typename R, typename S>
 inline Container sorted(const Container &&container, R S::*member)
 {
     return sorted(container, member);
 }
 
 // pointer to member function
-template <typename Container, typename R, typename S>
+template<typename Container, typename R, typename S>
 inline void sort(Container &container, R (S::*function)() const)
 {
     auto f = std::mem_fn(function);
@@ -1092,7 +1134,8 @@ inline void sort(Container &container, R (S::*function)() const)
     });
 }
 
-template <typename Container, typename R, typename S>
+// const lvalue
+template<typename Container, typename R, typename S>
 inline Container sorted(const Container &container, R (S::*function)() const)
 {
     Container c = container;
@@ -1100,14 +1143,27 @@ inline Container sorted(const Container &container, R (S::*function)() const)
     return c;
 }
 
-template <typename Container, typename R, typename S>
+// non-const lvalue
+// This is needed because otherwise the "universal" reference below is used, modifying the input
+// container.
+template<typename Container, typename R, typename S>
+inline Container sorted(Container &container, R (S::*function)() const)
+{
+    Container c = container;
+    sort(c, function);
+    return c;
+}
+
+// non-const rvalue (actually rvalue or lvalue, but lvalue is handled above)
+template<typename Container, typename R, typename S>
 inline Container sorted(Container &&container, R (S::*function)() const)
 {
     sort(container, function);
-    return container;
+    return std::move(container);
 }
 
-template <typename Container, typename R, typename S>
+// const rvalue
+template<typename Container, typename R, typename S>
 inline Container sorted(const Container &&container, R (S::*function)() const)
 {
     return sorted(container, function);
diff --git a/tests/auto/algorithm/tst_algorithm.cpp b/tests/auto/algorithm/tst_algorithm.cpp
index 5f2eaf23e2f..dbe3d732a47 100644
--- a/tests/auto/algorithm/tst_algorithm.cpp
+++ b/tests/auto/algorithm/tst_algorithm.cpp
@@ -28,6 +28,7 @@ private slots:
     void findOrDefault();
     void toReferences();
     void take();
+    void sorted();
 };
 
 
@@ -550,6 +551,106 @@ void tst_Algorithm::take()
     }
 }
 
+void tst_Algorithm::sorted()
+{
+    const QList<int> vOrig{4, 3, 6, 5, 8};
+    const QList<int> vExpected{3, 4, 5, 6, 8};
+
+    // plain
+    {
+        // non-const lvalue
+        QList<int> vncl = vOrig;
+        const QList<int> rncl = Utils::sorted(vncl);
+        QCOMPARE(rncl, vExpected);
+        QCOMPARE(vncl, vOrig); // was not modified
+
+        // const lvalue
+        const QList<int> rcl = Utils::sorted(vOrig);
+        QCOMPARE(rcl, vExpected);
+
+        // non-const rvalue
+        const auto vncr = [vOrig]() -> QList<int> { return vOrig; };
+        const QList<int> rncr = Utils::sorted(vncr());
+        QCOMPARE(rncr, vExpected);
+
+        // const rvalue
+        const auto vcr = [vOrig]() -> const QList<int> { return vOrig; };
+        const QList<int> rcr = Utils::sorted(vcr());
+        QCOMPARE(rcr, vExpected);
+    }
+
+    // predicate
+    {
+        // non-const lvalue
+        QList<int> vncl = vOrig;
+        const QList<int> rncl = Utils::sorted(vncl, [](int a, int b) { return a < b; });
+        QCOMPARE(rncl, vExpected);
+        QCOMPARE(vncl, vOrig); // was not modified
+
+        // const lvalue
+        const QList<int> rcl = Utils::sorted(vOrig, [](int a, int b) { return a < b; });
+        QCOMPARE(rcl, vExpected);
+
+        // non-const rvalue
+        const auto vncr = [vOrig]() -> QList<int> { return vOrig; };
+        const QList<int> rncr = Utils::sorted(vncr(), [](int a, int b) { return a < b; });
+        QCOMPARE(rncr, vExpected);
+
+        // const rvalue
+        const auto vcr = [vOrig]() -> const QList<int> { return vOrig; };
+        const QList<int> rcr = Utils::sorted(vcr(), [](int a, int b) { return a < b; });
+        QCOMPARE(rcr, vExpected);
+    }
+
+    const QList<Struct> mvOrig({4, 3, 2, 1});
+    const QList<Struct> mvExpected({1, 2, 3, 4});
+    // member
+    {
+        // non-const lvalue
+        QList<Struct> mvncl = mvOrig;
+        const QList<Struct> rncl = Utils::sorted(mvncl, &Struct::member);
+        QCOMPARE(rncl, mvExpected);
+        QCOMPARE(mvncl, mvOrig); // was not modified
+
+        // const lvalue
+        const QList<Struct> rcl = Utils::sorted(mvOrig, &Struct::member);
+        QCOMPARE(rcl, mvExpected);
+
+        // non-const rvalue
+        const auto vncr = [mvOrig]() -> QList<Struct> { return mvOrig; };
+        const QList<Struct> rncr = Utils::sorted(vncr(), &Struct::member);
+        QCOMPARE(rncr, mvExpected);
+
+        // const rvalue
+        const auto vcr = [mvOrig]() -> const QList<Struct> { return mvOrig; };
+        const QList<Struct> rcr = Utils::sorted(vcr(), &Struct::member);
+        QCOMPARE(rcr, mvExpected);
+    }
+
+    // member function
+    {
+        // non-const lvalue
+        QList<Struct> mvncl = mvOrig;
+        const QList<Struct> rncl = Utils::sorted(mvncl, &Struct::getMember);
+        QCOMPARE(rncl, mvExpected);
+        QCOMPARE(mvncl, mvOrig); // was not modified
+
+        // const lvalue
+        const QList<Struct> rcl = Utils::sorted(mvOrig, &Struct::getMember);
+        QCOMPARE(rcl, mvExpected);
+
+        // non-const rvalue
+        const auto vncr = [mvOrig]() -> QList<Struct> { return mvOrig; };
+        const QList<Struct> rncr = Utils::sorted(vncr(), &Struct::getMember);
+        QCOMPARE(rncr, mvExpected);
+
+        // const rvalue
+        const auto vcr = [mvOrig]() -> const QList<Struct> { return mvOrig; };
+        const QList<Struct> rcr = Utils::sorted(vcr(), &Struct::getMember);
+        QCOMPARE(rcr, mvExpected);
+    }
+}
+
 QTEST_GUILESS_MAIN(tst_Algorithm)
 
 #include "tst_algorithm.moc"