From 07b89093604db618c15fe53bd04950922bb3469b Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Wed, 19 Mar 2025 22:08:21 +0100 Subject: [PATCH] QByteArray(View) comparisons: tame the overload set a bit QByteArray declares comparison operators against QByteArrayView. With the addition of the implicit conversion from QByteArray to std::string_view, those comparison operators are now ambiguous when comparing a QByteArray against a std::string_view. The overload set contains: 1) QByteArray<=>QByteArrayView, after converting the string_view to a QByteArrayView 2) string_view<=>string_view, after converting the QByteArray to string_view. This is part of the fallout of adding QByteArray->std::string_view implicit conversions. We accepted the risk of SICs, and here they are. The first operator should have never been added: comparing a QByteArray to a QByteArrayView is already possible using QByteArrayView's own comparison operators (QBAV<=>QBAV, after converting the QBA to it). Therefore, this commit removes it. The removal reveals two other problems: 1) there is a comparison defined between QByteArrayView and const char *. That comparison makes comparing QByteArrayView and QByteArray ambiguous, because QByteArray can convert to both QByteArrayView (thus finding the QBAV<=>QBAV comparisosn) and to const char * (thus finding the QBAV<=>const char * comparison). Again, the latter overloads should have never been added: const char * implicitly converts to QByteArrayView and therefore it's served by the QBAV<=>QBAV comparison operators. This gets removed too. 2) comparisons between QByteArray and types convertible to QByteArrayView (e.g. std::string) are no longer possible. These were never meant to be supported: heterogeneous string comparisons must first and foremost convert everything to a common view type (...QAnyStringView?), and only then the comparison can proceed. In general, we should aim at this setup: 1) Owning string classes should only have homogeneous comparisons. They might offer heterogeneous comparisons (say, QString <=> const char *) on a case-by-case basis. The must never declare comparisons against views (instead, view's own comparisons will take care of comparing an owning string agains a view!). Comparing different owning string types directly is not meant to be supported (e.g. QString <=> std::u16_string), as per above. 2) Views only have homogeneous comparisons. Everything compatible will convert to the view type for the comparison. Again, exceptions on a case-by-case basis (e.g. QStringView<=>const char *, protected by the NO_ASCII macro / ASCII_WARN / etc.). 3) We do not support compare against views coming from other libraries (e.g. QByteArrayView<=>std::string_view). One always risks an ambiguity, since both convert to each other. SG16 has rejected ideas of a "string view protocol"; Qt view classes implicitly build from compatible ranges (which likely includes any other string view class out there), which makes it a magnet for these ambiguities (say against a 3rd party view class that also does the same). This isn't a complete overhaul: there are still strings and views that don't follow the above and this commit isn't changing them yet. Fixes: QTBUG-134902 Change-Id: If181af1fb627490423ca1e467ecce7330f7445fb Reviewed-by: Thiago Macieira --- src/corelib/text/qbytearray.cpp | 2 +- src/corelib/text/qbytearray.h | 1 - src/corelib/text/qbytearrayview.h | 7 ---- src/corelib/text/qbytearrayview.qdoc | 2 - .../tst_qstringapisymmetry.cpp | 37 +++++++++++++++---- 5 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/corelib/text/qbytearray.cpp b/src/corelib/text/qbytearray.cpp index eee179ffa14..d731f371e75 100644 --- a/src/corelib/text/qbytearray.cpp +++ b/src/corelib/text/qbytearray.cpp @@ -832,7 +832,7 @@ QByteArray qUncompress(const uchar* data, qsizetype nbytes) \reentrant \compares strong - \compareswith strong {const char *} QByteArrayView + \compareswith strong {const char *} \endcompareswith \compareswith strong QChar char16_t QString QStringView QLatin1StringView \ QUtf8StringView diff --git a/src/corelib/text/qbytearray.h b/src/corelib/text/qbytearray.h index a8b456a73ef..cb445788f45 100644 --- a/src/corelib/text/qbytearray.h +++ b/src/corelib/text/qbytearray.h @@ -564,7 +564,6 @@ private: return Qt::compareThreeWay(res, 0); } Q_DECLARE_STRONGLY_ORDERED(QByteArray) - Q_DECLARE_STRONGLY_ORDERED(QByteArray, QByteArrayView) Q_DECLARE_STRONGLY_ORDERED(QByteArray, const char *) #if defined(__GLIBCXX__) && defined(__cpp_lib_three_way_comparison) // libstdc++ has a bug [0] when `operator const void *()` is preferred over diff --git a/src/corelib/text/qbytearrayview.h b/src/corelib/text/qbytearrayview.h index 568b69dbb3f..4f40fd12097 100644 --- a/src/corelib/text/qbytearrayview.h +++ b/src/corelib/text/qbytearrayview.h @@ -366,13 +366,6 @@ private: } Q_DECLARE_STRONGLY_ORDERED(QByteArrayView) - friend bool comparesEqual(const QByteArrayView &lhs, const char *rhs) noexcept - { return comparesEqual(lhs, QByteArrayView(rhs)); } - friend Qt::strong_ordering - compareThreeWay(const QByteArrayView &lhs, const char *rhs) noexcept - { return compareThreeWay(lhs, QByteArrayView(rhs)); } - Q_DECLARE_STRONGLY_ORDERED(QByteArrayView, const char *) - // defined in qstring.cpp friend Q_CORE_EXPORT bool comparesEqual(const QByteArrayView &lhs, const QChar &rhs) noexcept; diff --git a/src/corelib/text/qbytearrayview.qdoc b/src/corelib/text/qbytearrayview.qdoc index 108fa0cd59a..36d5743d61a 100644 --- a/src/corelib/text/qbytearrayview.qdoc +++ b/src/corelib/text/qbytearrayview.qdoc @@ -15,8 +15,6 @@ \reentrant \compares strong - \compareswith strong QByteArray {const char *} - \endcompareswith \compareswith strong QString QStringView QUtf8StringView QLatin1StringView \ QChar char16_t When comparing with string and Unicode character types, the content is diff --git a/tests/auto/corelib/text/qstringapisymmetry/tst_qstringapisymmetry.cpp b/tests/auto/corelib/text/qstringapisymmetry/tst_qstringapisymmetry.cpp index ad32c52ad78..0b2aaa8281e 100644 --- a/tests/auto/corelib/text/qstringapisymmetry/tst_qstringapisymmetry.cpp +++ b/tests/auto/corelib/text/qstringapisymmetry/tst_qstringapisymmetry.cpp @@ -123,7 +123,7 @@ private: // void compare_data(bool hasConceptOfNullAndEmpty=true); - template + template void compare_impl() const; private Q_SLOTS: @@ -184,6 +184,8 @@ private Q_SLOTS: void compare_QString_QByteArrayView() { compare_impl(); } void compare_QString_const_char_star_data() { compare_data(); } void compare_QString_const_char_star() { compare_impl(); } + void compare_QString_std_u16_string_view_data() { compare_data(); } + void compare_QString_std_u16_string_view() { compare_impl(); } void compare_QStringView_QChar_data() { compare_data(false); } void compare_QStringView_QChar() { compare_impl(); } @@ -203,6 +205,8 @@ private Q_SLOTS: void compare_QStringView_QByteArrayView() { compare_impl(); } void compare_QStringView_const_char_star_data() { compare_data(); } void compare_QStringView_const_char_star() { compare_impl(); } + void compare_QStringView_std_u16_string_data() { compare_data(); } + void compare_QStringView_std_u16_string() { compare_impl(); } void compare_QUtf8StringView_QChar_data() { compare_data(false); } void compare_QUtf8StringView_QChar() { compare_impl(); } @@ -260,6 +264,13 @@ private Q_SLOTS: void compare_QByteArray_QByteArrayView() { compare_impl(); } void compare_QByteArray_const_char_star_data() { compare_data(); } void compare_QByteArray_const_char_star() { compare_impl(); } + void compare_QByteArray_std_string_view_data() { compare_data(); } + void compare_QByteArray_std_string_view() + { +#ifdef QT_BYTEARRAY_CONVERTS_TO_STD_STRING_VIEW + compare_impl(); +#endif + } void compare_QByteArrayView_QChar_data() { compare_data(false); } void compare_QByteArrayView_QChar() { compare_impl(); } @@ -279,6 +290,8 @@ private Q_SLOTS: void compare_QByteArrayView_QByteArrayView() { compare_impl(); } void compare_QByteArrayView_const_char_star_data() { compare_data(); } void compare_QByteArrayView_const_char_star() { compare_impl(); } + void compare_QByteArrayView_std_string_data() { compare_data(); } + void compare_QByteArrayView_std_string() { compare_impl(); } void compare_const_char_star_QChar_data() { compare_data(false); } void compare_const_char_star_QChar() { compare_impl(); } @@ -1630,6 +1643,13 @@ MAKE(QUtf8StringView) { return u8; } MAKE(QAnyStringViewUsingL1) { return {QAnyStringView{l1}}; } MAKE(QAnyStringViewUsingU8) { return {QAnyStringView{u8}}; } MAKE(QAnyStringViewUsingU16) { return {QAnyStringView{sv}}; } +MAKE(std::string) { return u8.toStdString(); } +#ifdef QT_BYTEARRAY_CONVERTS_TO_STD_STRING_VIEW +MAKE(std::string_view) { return u8; } +#else +MAKE(std::string_view) { return std::string_view(u8.data(), size_t(u8.size())); } +#endif +MAKE(std::u16string_view) { return sv; } #undef MAKE // Some types have ASCII-only case-insensitive compare, but are handled as containing @@ -1642,7 +1662,7 @@ template <> constexpr bool is_bytearray_like_v = true; template constexpr bool has_nothrow_member_compare_v = is_bytearray_like_v == is_bytearray_like_v; -template +template void tst_QStringApiSymmetry::compare_impl() const { QFETCH(QStringView, lhsUnicode); @@ -1686,11 +1706,14 @@ void tst_QStringApiSymmetry::compare_impl() const CHECK(<=); CHECK(>=); #undef CHECK - // Test that all string-like types implemente compareThreeWay() as a friend - // function. - const Qt::strong_ordering expectedOrdering = - Qt::compareThreeWay(caseSensitiveCompareResult, 0); - QCOMPARE_EQ(qCompareThreeWay(lhs, rhs), expectedOrdering); + + if constexpr (CheckCompareThreeWay) { + // Test that all string-like types implemente compareThreeWay() as a friend + // function. + const Qt::strong_ordering expectedOrdering = + Qt::compareThreeWay(caseSensitiveCompareResult, 0); + QCOMPARE_EQ(qCompareThreeWay(lhs, rhs), expectedOrdering); + } } template