[Development] C++20 comparisons @ Qt (was: Re: C++20 @ Qt)

[Ivan Solovev]

> Ah, I see. It's not an ambiguous overload problem because the hidden friend is
> not even part of the overload set in the first place. That can be solved by
> promoting the function from hidden friend to static member, either as public
> or private, depending on the implementer's choice.
>
> That means it can't be used in ADL contexts without scope qualifications, but I
> don't see that as an API requirement. The API should be the operators.

We introduce new public macros, which means that users will be using them in
their custom classes in order to achieve the same unified comparison behavior
between C++17 and C++20.
This means that the users will have to implement the helper functions required
for these macros. I agree that for the equal()/equals() helper function, the
op==() and op!=() are mostly usable.
But what about the order()/compare() function? I assume that the users would
NOT be able to use op<=>(), because they will write C++17-compatible code.
And testing for Unordered in terms of C++17 relational operators looks like an
unnecessarily large amount of code, especially if we already have written this
code for them. So why not just let them use it?
Of course, you can argue that they can use the public APIs directly (if they
exist). But what if the user develops a generic class?

What we can do is to have both private/public static two-arg member and a
hidden friend, which would simply call the static method. We will have to
apply this hack only to the classes that already have public APIs with the
clashing names. For other classes we can just use hidden friends directly.

> You forgot the <=> 0 here. Comparing an int with 0 through the spaceship
> operator produces a std::strong_ordering result. And comparing a
> std::strong_ordering with 0 via the spaceship returns itself too.
>
> https://gcc.godbolt.org/z/ebKe8Eb3a
>
> This works for weak_ordering too. For partial_ordering, the case of unordered
> needs to be handled explicitly, so I'd instead require that the called
> function return either std::partial_ordering or QPartialOrdering, nothing
> else.

Well, the idea was to avoid unnecessary operations, and basically optimize
operator<=>() to simply call the helper function in C++20 case.

But anyway, that's a nice approach, and I should consider using it in my
patches. Specially if it allows us to "unblock" compare() as a name for the
helper function.

------------------------------

Ivan Solovev
Senior Software Engineer

The Qt Company GmbH
Erich-Thilo-Str. 10
12489 Berlin, Germany
ivan.solovev at qt.io
www.qt.io

Geschäftsführer: Mika Pälsi,
Juha Varelius, Jouni Lintunen
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht
Charlottenburg, HRB 144331 B

________________________________
From: Thiago Macieira
Sent: Tuesday, July 25, 2023 4:42 PM
To: development at qt-project.org
Cc: Ivan Solovev
Subject: Re: [Development] C++20 comparisons @ Qt (was: Re: C++20 @ Qt)

On Tuesday, 25 July 2023 01:42:42 PDT Ivan Solovev via Development wrote:
> > And why is that a problem? So long as the proper overload exists, it'll be
> > called. There's only a problem if the overloads are ambiguous.
>
> The problem is that there might be no "proper" overload, but the compiler
> would still try to use the public API methods, ignoring the hidden friend
> with a correct signature. That's what I tried to illustrate with the links
> from my previous email.

Ah, I see. It's not an ambiguous overload problem because the hidden friend is
not even part of the overload set in the first place. That can be solved by
promoting the function from hidden friend to static member, either as public
or private, depending on the implementer's choice.

That means it can't be used in ADL contexts without scope qualifications, but I
don't see that as an API requirement. The API should be the operators.

> Also, we have inconsistency among our classes, as some of them use two-arg
> static equals(), and others use one-arg non-static equals().

That doesn't seem to be a problem. If we can't make the macros work with
either, then we can simply add another equals to match the one we want,
adapting to the one we don't. That is, we'd probably add a static equals that
calls the non-static one if the latter is already public API and is in the
ABI.

> In my patch[0] I introduce several helper macros that use helper functions
> to provide the actual implementation.
> Since we are talking about "equal()/equals()", let's take
> Q_DECLARE_EQUALITY_COMPARABLE as an example. Omitting all the details,
> and supposing that we use "equals" as a helepr function name, it expands
> into something like this:
>
>     friend bool operator==(const MyType &lhs, const MyType &rhs) noexcept
>     { return equals(lhs, rhs); }
>
> As you can see, we selected the two-arg version of the helper function.
> This will work without any additional changes for the classes like
> QBluetoothDeviceInfo, which has a private static two-arg equals[1], but
> would fail for classes like QLocale, because it has a one-arg non-static
> equals[2].

As above: let's just add the static bool equals(two arg) to QLocale:

 private:
     QLocale(QLocalePrivate &dd);
     bool equals(const QLocale &other) const;
+    static bool equals(const QLocale &l1, const QLocale &l2)
+    { return l1.equals(l2); }
     friend class QLocalePrivate;
     friend class QSystemLocale;

The ABI for either function is the same and only differ in mangling. Since
QLocale is not trivially copyable, we can't implement this with pass-by-value.

> And in any case, none of the existing public APIs would be able to work with
> the generic code. And I keep repeating that it's something that we want to
> provide for our users as building blocks for implementing
> three-way-comparison with C++17.

I disagree with that requirement *because* it's costly. Let them use <=> if
they want to use generic code.

> The problem with "compare" is the pre-existing QString::compare(), which
> returns an int.

That is not a problem. int is a suitable strong order result type.

> Again, let's consider an example macro from [0].
> Let's take Q_DECLARE_STRONGLY_ORDERED, which expands into all 6 relational
> operators under C++17. I'll consider only operator>() and operator<() here
> for simplicity:
>
>     friend bool operator<(const MyType &lhs, const MyType &rhs) noexcept
>     { return order(lhs, rhs) == QStrongOrdering::Less; }
>     friend bool operator>(const MyType &lhs, const MyType &rhs) noexcept
>     { return order(lhs, rhs) == QStrongOrdering::Greater; }

I had posted a comment months ago that this definition was wrong, as you'd
noticed:

>     friend bool operator<(const MyType &lhs, const MyType &rhs) noexcept
>     { return compare(lhs, rhs) < 0; }
>     friend bool operator>(const MyType &lhs, const MyType &rhs) noexcept
>     { return compare(lhs, rhs) > 0; }
>
> HOWEVER, that will work ONLY for C++17.
>
> For C++20 we want the Q_DECLARE_STRONGLY_ORDERED marco to simply expand into
> operator<=>():
>
>     friend auto operator<=>(const MyType &lhs, const MyType &rhs) noexcept
>     { return order(lhs, rhs); }

You forgot the <=> 0 here. Comparing an int with 0 through the spaceship
operator produces a std::strong_ordering result. And comparing a
std::strong_ordering with 0 via the spaceship returns itself too.

https://gcc.godbolt.org/z/ebKe8Eb3a

This works for weak_ordering too. For partial_ordering, the case of unordered
needs to be handled explicitly, so I'd instead require that the called
function return either std::partial_ordering or QPartialOrdering, nothing
else.

--
Thiago Macieira - thiago.macieira (AT) intel.com
  Cloud Software Architect - Intel DCAI Cloud Engineering
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20230726/7a103b44/attachment-0001.htm>