Port QModelIndex to new comparison helper macros

Use Milian's asTuple() trick and compareThreeWayMulti().

The pointer variable presents a challenge, because, like op<, op<=>
cannot order unrelated pointer values. Trying to is UB; one is
supposed to use std::less and std::compare_three_way instead, but, you
guessed it, less<tuple<T*>> uses op< to compare the tuples, which, in
turn uses op< on the T* member, causing UB.

[ChangeLog][QtCore][QtCompare] Added a Qt::totally_ordered_wrapper
class whose relational operators use the std ordering types. Use
it to wrap the pointers when doing the comparison to avoid UB.

This allows a smooth migration once we depend on C++20 and class
authors start using lhs.asTuple() <=> rhs.asTuple() or even start
to =default op<=>. The wrapper type is robust against all these
transformations.

Update QModelIndex comparison documentation part.

Task-number: QTBUG-117660
Change-Id: I126b2ebff458800134ed6e774a389d85d76a395f
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>

src/corelib/global/qcomparehelpers.h

@@ -559,9 +559,103 @@ constexpr Qt::strong_ordering compareThreeWay(Enum lhs, Enum rhs) noexcept
 {
     return compareThreeWay(qToUnderlying(lhs), qToUnderlying(rhs));
 }
-
 } // namespace Qt
 
+namespace QtOrderingPrivate {
+
+template <typename Head, typename...Tail, std::size_t...Is>
+constexpr std::tuple<Tail...> qt_tuple_pop_front_impl(const std::tuple<Head, Tail...> &t,
+                                                      std::index_sequence<Is...>) noexcept
+{
+    return std::tuple<Tail...>(std::get<Is + 1>(t)...);
+}
+
+template <typename Head, typename...Tail>
+constexpr std::tuple<Tail...> qt_tuple_pop_front(const std::tuple<Head, Tail...> &t) noexcept
+{
+    return qt_tuple_pop_front_impl(t, std::index_sequence_for<Tail...>{});
+}
+
+template <typename LhsHead, typename...LhsTail, typename RhsHead, typename...RhsTail>
+constexpr auto compareThreeWayMulti(const std::tuple<LhsHead, LhsTail...> &lhs, // ie. not empty
+                                    const std::tuple<RhsHead, RhsTail...> &rhs) noexcept
+{
+    static_assert(sizeof...(LhsTail) == sizeof...(RhsTail),
+                                                            // expanded together below, but provide a nicer error message:
+                  "The tuple arguments have to have the same size.");
+
+    using Qt::compareThreeWay;
+    using R = std::common_type_t<
+            decltype(compareThreeWay(std::declval<LhsHead>(), std::declval<RhsHead>())),
+            decltype(compareThreeWay(std::declval<LhsTail>(), std::declval<RhsTail>()))...
+            >;
+
+    const auto &l = std::get<0>(lhs);
+    const auto &r = std::get<0>(rhs);
+    static_assert(noexcept(compareThreeWay(l, r)),
+                  "This function requires all relational operators to be noexcept.");
+    const auto res = compareThreeWay(l, r);
+    if constexpr (sizeof...(LhsTail) > 0) {
+        if (is_eq(res))
+            return R{compareThreeWayMulti(qt_tuple_pop_front(lhs), qt_tuple_pop_front(rhs))};
+    }
+    return R{res};
+}
+
+} //QtOrderingPrivate
+
+namespace Qt {
+// A wrapper class that adapts the wrappee to use the strongly-ordered
+// <functional> function objects for implementing the relational operators.
+// Mostly useful to avoid UB on pointers (which it currently mandates P to be),
+// because all the comparison helpers (incl. std::compare_three_way on
+// std::tuple<T*>!) will use the language-level operators.
+//
+template <typename P>
+class totally_ordered_wrapper
+{
+    static_assert(std::is_pointer_v<P>);
+    using T = std::remove_pointer_t<P>;
+
+    P ptr;
+public:
+    explicit constexpr totally_ordered_wrapper(P p) : ptr(p) {}
+
+    constexpr P get() const noexcept { return ptr; }
+    constexpr P operator->() const noexcept { return get(); }
+    constexpr T& operator*() const noexcept { return *get(); }
+
+    explicit constexpr operator bool() const noexcept { return get(); }
+
+private:
+    friend constexpr auto compareThreeWay(const totally_ordered_wrapper &lhs, const totally_ordered_wrapper &rhs) noexcept
+    { return Qt::compareThreeWay(lhs.ptr, rhs.ptr); }
+#define MAKE_RELOP(Ret, op, Op) \
+    friend constexpr Ret operator op (const totally_ordered_wrapper &lhs, const totally_ordered_wrapper &rhs) noexcept \
+    { return std:: Op {}(lhs.ptr, rhs.ptr); } \
+    friend constexpr Ret operator op (const totally_ordered_wrapper &lhs, const P &rhs) noexcept \
+    { return std:: Op {}(lhs.ptr, rhs    ); } \
+    friend constexpr Ret operator op (const P &lhs, const totally_ordered_wrapper &rhs) noexcept \
+    { return std:: Op {}(lhs,     rhs.ptr); } \
+    friend constexpr Ret operator op (const totally_ordered_wrapper &lhs, std::nullptr_t) noexcept \
+    { return std:: Op {}(lhs.ptr, nullptr); } \
+    friend constexpr Ret operator op (std::nullptr_t, const totally_ordered_wrapper &rhs) noexcept \
+    { return std:: Op {}(nullptr, rhs.ptr); } \
+    /* end */
+    MAKE_RELOP(bool, ==, equal_to<P>)
+    MAKE_RELOP(bool, !=, not_equal_to<P>)
+    MAKE_RELOP(bool, < , less<P>)
+    MAKE_RELOP(bool, <=, less_equal<P>)
+    MAKE_RELOP(bool, > , greater<P>)
+    MAKE_RELOP(bool, >=, greater_equal<P>)
+#ifdef __cpp_lib_three_way_comparison
+    MAKE_RELOP(auto, <=>, compare_three_way)
+#endif
+#undef MAKE_RELOP
+};
+
+} //Qt
+
 QT_END_NAMESPACE
 
 #endif // QCOMPAREHELPERS_H

src/corelib/itemmodels/qabstractitemmodel.cpp

@@ -1158,6 +1158,7 @@ void QAbstractItemModel::resetInternalData()
 
     \ingroup model-view
 
+    \compares strong
 
     This class is used as an index into item models derived from
     QAbstractItemModel. The index is used by item views, delegates, and
@@ -1328,24 +1329,22 @@ void QAbstractItemModel::resetInternalData()
 */
 
 /*!
-    \fn bool QModelIndex::operator==(const QModelIndex &other) const
+    \fn bool QModelIndex::operator==(const QModelIndex &lhs, const QModelIndex &rhs)
 
-    Returns \c{true} if this model index refers to the same location as the
+    Returns \c{true} if \a lhs model index refers to the same location as the
-    \a other model index; otherwise returns \c{false}.
+    \a rhs model index; otherwise returns \c{false}.
 
     The internal data pointer, row, column, and model values are used when
     comparing with another model index.
 */
 
-
 /*!
-    \fn bool QModelIndex::operator!=(const QModelIndex &other) const
+    \fn bool QModelIndex::operator!=(const QModelIndex &lhs, const QModelIndex &rhs)
 
-    Returns \c{true} if this model index does not refer to the same location as
+    Returns \c{true} if \a lhs model index does not refer to the same location as
-    the \a other model index; otherwise returns \c{false}.
+    the \a rhs model index; otherwise returns \c{false}.
 */
 
-
 /*!
     \fn QModelIndex QModelIndex::parent() const
 
@@ -4124,10 +4123,10 @@ bool QAbstractListModel::dropMimeData(const QMimeData *data, Qt::DropAction acti
 */
 
 /*!
-    \fn bool QModelIndex::operator<(const QModelIndex &other) const
+    \fn bool QModelIndex::operator<(const QModelIndex &lhs, const QModelIndex &rhs)
     \since 4.1
 
-    Returns \c{true} if this model index is smaller than the \a other
+    Returns \c{true} if \a lhs model index is smaller than the \a rhs
     model index; otherwise returns \c{false}.
 
     The less than calculation is not directly useful to developers - the way that indexes

src/corelib/itemmodels/qabstractitemmodel.h

@@ -5,11 +5,14 @@
 #ifndef QABSTRACTITEMMODEL_H
 #define QABSTRACTITEMMODEL_H
 
+#include <QtCore/qcompare.h>
 #include <QtCore/qhash.h>
 #include <QtCore/qlist.h>
 #include <QtCore/qobject.h>
 #include <QtCore/qvariant.h>
 
+#include <tuple>
+
 QT_REQUIRE_CONFIG(itemmodel);
 
 QT_BEGIN_NAMESPACE
@@ -138,19 +141,16 @@ public:
     inline QVariant data(int role = Qt::DisplayRole) const;
     inline void multiData(QModelRoleDataSpan roleDataSpan) const;
     inline Qt::ItemFlags flags() const;
-    constexpr inline const QAbstractItemModel *model() const noexcept { return m; }
+    constexpr inline const QAbstractItemModel *model() const noexcept { return m.get(); }
     constexpr inline bool isValid() const noexcept { return (r >= 0) && (c >= 0) && (m != nullptr); }
-    constexpr inline bool operator==(const QModelIndex &other) const noexcept
+
-        { return (other.r == r) && (other.i == i) && (other.c == c) && (other.m == m); }
+private:
-    constexpr inline bool operator!=(const QModelIndex &other) const noexcept
+    constexpr auto asTuple() const noexcept { return std::tie(r, c, i, m); }
-        { return !(*this == other); }
+    friend constexpr bool comparesEqual(const QModelIndex &lhs, const QModelIndex &rhs) noexcept
-    constexpr inline bool operator<(const QModelIndex &other) const noexcept
+    { return lhs.asTuple() == rhs.asTuple(); }
-        {
+    friend constexpr Qt::strong_ordering compareThreeWay(const QModelIndex &lhs, const QModelIndex &rhs) noexcept
-            return  r <  other.r
+    { return QtOrderingPrivate::compareThreeWayMulti(lhs.asTuple(), rhs.asTuple()); }
-                || (r == other.r && (c <  other.c
+    Q_DECLARE_STRONGLY_ORDERED_LITERAL_TYPE(QModelIndex)
-                                 || (c == other.c && (i <  other.i
-                                                  || (i == other.i && std::less<const QAbstractItemModel *>()(m, other.m))))));
-        }
 private:
     inline QModelIndex(int arow, int acolumn, const void *ptr, const QAbstractItemModel *amodel) noexcept
         : r(arow), c(acolumn), i(reinterpret_cast<quintptr>(ptr)), m(amodel) {}
@@ -158,7 +158,7 @@ private:
         : r(arow), c(acolumn), i(id), m(amodel) {}
     int r, c;
     quintptr i;
-    const QAbstractItemModel *m;
+    Qt::totally_ordered_wrapper<const QAbstractItemModel *> m;
 };
 Q_DECLARE_TYPEINFO(QModelIndex, Q_RELOCATABLE_TYPE);
 

tests/auto/corelib/itemmodels/qabstractitemmodel/tst_qabstractitemmodel.cpp

@@ -993,7 +993,7 @@ void tst_QAbstractItemModel::complexChangesWithPersistent()
 
 void tst_QAbstractItemModel::modelIndexComparisons()
 {
-    QTestPrivate::testEqualityOperatorsCompile<QModelIndex>();
+    QTestPrivate::testAllComparisonOperatorsCompile<QModelIndex>();
     QTestPrivate::testEqualityOperatorsCompile<QPersistentModelIndex>();
     QTestPrivate::testEqualityOperatorsCompile<QPersistentModelIndex, QModelIndex>();
 
@@ -1006,6 +1006,9 @@ void tst_QAbstractItemModel::modelIndexComparisons()
 
     QT_TEST_EQUALITY_OPS(mi11, mi11, true);
     QT_TEST_EQUALITY_OPS(mi11, mi22, false);
+    QT_TEST_ALL_COMPARISON_OPS(mi11, mi11, Qt::strong_ordering::equal);
+    QT_TEST_ALL_COMPARISON_OPS(mi11, mi22, Qt::strong_ordering::less);
+    QT_TEST_ALL_COMPARISON_OPS(mi22, mi11, Qt::strong_ordering::greater);
     QT_TEST_EQUALITY_OPS(pmi11, pmi11, true);
     QT_TEST_EQUALITY_OPS(pmi11, pmi22, false);
     QT_TEST_EQUALITY_OPS(pmi11, mi11, true);