From 8a058202edb20e7e308221f19594a536e1e761d1 Mon Sep 17 00:00:00 2001
From: Mat Sutcliffe <oktal3700@gmail.com>
Date: Fri, 30 Nov 2018 15:50:52 +0000
Subject: [PATCH] Fixed buggy CSequence::sortBy when sorting by multiple
 members.

Implementation of Predicates::MemberLess was wrong, so replaced it with a
simpler one. Also added a regression test and removed unused code.
---
 src/blackmisc/algorithm.h                     | 21 -------------
 src/blackmisc/predicates.h                    | 14 ++-------
 .../testcontainers/testcontainers.cpp         | 30 +++++++++++++++++++
 3 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/src/blackmisc/algorithm.h b/src/blackmisc/algorithm.h
index b1742a228..df6a84540 100644
--- a/src/blackmisc/algorithm.h
+++ b/src/blackmisc/algorithm.h
@@ -144,17 +144,6 @@ namespace BlackMisc
 
     namespace Private
     {
-        //! \private
-        template <typename T, typename F, size_t... Is>
-        void tupleForEachImpl(T &&tuple, F &&visitor, std::index_sequence<Is...>)
-        {
-            // parameter pack swallow idiom
-            static_cast<void>(std::initializer_list<int>
-            {
-                //! \fixme C-style cast is needed due to a clang bug: https://bugs.llvm.org/show_bug.cgi?id=39375
-                ((void)(std::forward<F>(visitor)(std::get<Is>(std::forward<T>(tuple)))), 0)...
-            });
-        }
         //! \private
         template <typename T, typename F, size_t... Is>
         void tupleForEachPairImpl(T &&tuple, F &&visitor, std::index_sequence<Is...>)
@@ -168,16 +157,6 @@ namespace BlackMisc
         }
     }
 
-    /*!
-     * Invoke a visitor function on each element of a tuple in order.
-     */
-    template <typename T, typename F>
-    void tupleForEach(T &&tuple, F &&visitor)
-    {
-        using seq = std::make_index_sequence<std::tuple_size<std::decay_t<T>>::value>;
-        return Private::tupleForEachImpl(std::forward<T>(tuple), std::forward<F>(visitor), seq());
-    }
-
     /*!
      * Invoke a visitor function on each pair of elements of a tuple in order.
      */
diff --git a/src/blackmisc/predicates.h b/src/blackmisc/predicates.h
index 88f839e98..e3e96444e 100644
--- a/src/blackmisc/predicates.h
+++ b/src/blackmisc/predicates.h
@@ -62,15 +62,7 @@ namespace BlackMisc
         {
             return [vs...](const auto &a, const auto &b)
             {
-                bool less = true;
-                bool greater = false;
-                tupleForEach(std::make_tuple(vs...), [ & ](auto member)
-                {
-                    less = less && ! greater && (a.*member)() < (b.*member)();
-                    greater = (b.*member)() < (a.*member)();
-                });
-                Q_UNUSED(greater); // CPP style check
-                return less;
+                return std::forward_as_tuple((a.*vs)()...) < std::forward_as_tuple((b.*vs)()...);
             };
         }
 
@@ -119,9 +111,7 @@ namespace BlackMisc
         {
             return [vs...](const auto &a, const auto &b)
             {
-                bool equal = true;
-                tupleForEach(std::make_tuple(vs...), [ & ](auto member) { equal = equal && (a.*member)() == (b.*member)(); });
-                return equal;
+                return std::forward_as_tuple((a.*vs)()...) == std::forward_as_tuple((b.*vs)()...);
             };
         }
 
diff --git a/tests/blackmisc/testcontainers/testcontainers.cpp b/tests/blackmisc/testcontainers/testcontainers.cpp
index ca9b2e6a3..e478d8975 100644
--- a/tests/blackmisc/testcontainers/testcontainers.cpp
+++ b/tests/blackmisc/testcontainers/testcontainers.cpp
@@ -64,6 +64,7 @@ namespace BlackMiscTest
         void sequenceBasics();
         void joinAndSplit();
         void findTests();
+        void sortTests();
         void dictionaryBasics();
         void timestampList();
         void offsetTimestampList();
@@ -169,6 +170,35 @@ namespace BlackMiscTest
         QVERIFY2(found.size() == 1, "found");
     }
 
+    void CTestContainers::sortTests()
+    {
+        struct Person
+        {
+            const QString &getName() const { return name; }
+            int getAge() const { return age; }
+            bool operator==(const Person &other) const { return name == other.name && age == other.age; }
+            QString name;
+            int age;
+        };
+        CSequence<Person> list
+        {
+            { "Alice", 33 },
+            { "Bob", 32 },
+            { "Cathy", 32 },
+            { "Dave", 31 },
+            { "Emily", 31 }
+        };
+        CSequence<Person> sorted
+        {
+            { "Dave", 31 },
+            { "Emily", 31 },
+            { "Bob", 32 },
+            { "Cathy", 32 },
+            { "Alice", 33 }
+        };
+        QVERIFY2(list.sortedBy(&Person::getAge, &Person::getName) == sorted, "sort by multiple members");
+    }
+
     void CTestContainers::dictionaryBasics()
     {
         CTestValueObject key1("Key1", "This is key object 1");