Page MenuHomePhabricator

[C++20][Clang] P2468R2 The Equality Operator You Are Looking For
ClosedPublic

Authored by usaxena95 on Sep 23 2022, 6:18 AM.

Details

Summary

Implement
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2468r2.html.

Primarily we now accept

template<typename T> struct CRTPBase {
  bool operator==(const T&) const;
  bool operator!=(const T&) const;
};
struct CRTP : CRTPBase<CRTP> {};
bool cmp_crtp = CRTP() == CRTP();
bool cmp_crtp2 = CRTP() != CRTP();

Fixes: https://github.com/llvm/llvm-project/issues/57711

Diff Detail

Event Timeline

usaxena95 created this revision.Sep 23 2022, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2022, 6:18 AM
usaxena95 requested review of this revision.Sep 23 2022, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2022, 6:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
usaxena95 edited the summary of this revision. (Show Details)Sep 23 2022, 6:23 AM
usaxena95 added a reviewer: ilya-biryukov.
usaxena95 added a subscriber: Restricted Project.
usaxena95 edited the summary of this revision. (Show Details)Sep 23 2022, 11:44 AM
usaxena95 updated this revision to Diff 463237.Sep 27 2022, 8:06 AM

Added better diagnostic for non-const symmetric operator==.
Added release notes.
Updated tests.

usaxena95 edited the summary of this revision. (Show Details)Sep 27 2022, 8:11 AM
ilya-biryukov added a comment.EditedSep 27 2022, 9:03 AM

A few ideas for tests:

  • Static operators. Technically disallowed by the standard, but Clang seems to recover without dropping the member, so it seems we can actually look it up.
struct X { 
  bool operator ==(X const&) const;
  static bool operator !=(X const&, X const&);
};

this could potentially lead to a crash if the parameter list for the operator is empty:

struct X { 
  bool operator ==(X const&) const;
  static bool operator !=();
};
  • template functions with non-matching template heads (so they don't "correspond")
template <class T, class U = int>
bool operator ==(T, T);
template <class T>
bool operator !=(T, T)
  • template function and non-template function:
template <class T= int>
bool operator ==(X, X);
bool operator !=(X, X);
clang/include/clang/Sema/Sema.h
3768 ↗(On Diff #462464)

This looks like a leftover from previous version. Remove?

clang/lib/Sema/SemaOverload.cpp
893

Why do we need to move this from OperatorRewriteInfo to OverloadCandidateSet?

970

We only use Args[1], let's pass Expr* instead of an array.
The standard calls this expression "first operand", so let's sync the naming and accept the Expr *FirstOperand?

971

NIT: maybe rename to EqFD?
EqEqFD is somewhat hard to grasp and I don't think there any possibility for confusion with assignment here.

990

NIT: use early exits:

auto *RHSRec = RHS->getAs<RecordType>();
if (!RHSRec)
  return true;
// <else branch with reduced nesting>
996

Could we implement the "corresponds" check from (basic.scope.scope)p4 directly?

This should address the existing FIXMEs about const members and template functions.

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
142

NIT: the rest of the file uses plain comments. Maybe stick to existing style and avoid heading padded with =====

ilya-biryukov added a reviewer: Restricted Project.Sep 27 2022, 9:22 AM

Overall LG, thanks!
The only major comment from me is that we probably want to implement the full "corresponds" check so we handle various cases mentioned in the FIXMEs.

Also adding the Language WG as reviewers in case someone else wants to take a look.

I don't follow why this disallows the reverse #4.

It seems the trick is to pick the right type of the lookup.
The particular wording says to search for a name in scope rather than do a qualified lookup.
Only the latter should look inside the qualified namespaces.

Another idea for a test: try replacing declarations of operator!= with using other_ns::operator!= in various examples and make sure that it keeps working.

usaxena95 updated this revision to Diff 464010.Sep 29 2022, 1:34 PM
usaxena95 marked 7 inline comments as done.

Addressed comments.

usaxena95 updated this revision to Diff 464012.Sep 29 2022, 1:40 PM

Added test for static operator.

usaxena95 added inline comments.Sep 29 2022, 1:42 PM
clang/lib/Sema/SemaOverload.cpp
893

We are using operator location for search which is part of OverloadCandidateSet.
I propagated it to the RewriteInfo without shuffling these declarations.

996

Implemented the corresponds and search through LookupName.
Needs additional filtering based on matching DeclContext to verify they are from the same namespace.

I was not able to find any utility which facilitates "search" out of the box. Does this look good ?

Thanks! I have only various code style comments here, ptal. Overall LG.

clang/include/clang/Basic/DiagnosticSemaKinds.td
4707

NIT: for consistency with other diagnostics.

clang/include/clang/Sema/Overload.h
1033–1035

I am not sure the new name adds clarity.
It's unclear what the true return value means here. should clearly indicated returning true means the code has to proceed with adding the reversed operator. may means the code can choose to do so or not, I don't think that's quite right. should was really a better choice here.

That said, I don't think the rename itself is a bad idea, maybe there is a better name, but I couldn't come up with one.

clang/lib/Sema/SemaOverload.cpp
904

Or, wow, I did not realize that template parameters types for different functions are considered equal. TIL something new, thanks :)

953

NIT: remove const_cast, the non-const FunctionDecl* instead.
You'd need to do this for the corresponding parameter in shouldAddReversed too, but that seems very easy.

975–977

NIT: same suggestion as before. Just pass Expr* FirstOperand as the parameter instead of an array.

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
129

NIT: same suggestion as before. use plain comments without ==== padding for consistency with the rest of the file.

207

NIT: could we match against a larger piece of the message?
it's hard to read what the test is trying to check here.
In particular, I'm not sure if this intends to show the newly added note or simply point to operator that had its arguments reversed.

231

NIT: could you add a comment explaining why this is ambiguous? This seems non-obvious.
It's because the search for operator!= happens inside B and never finds 3, right?

238

NIT: could you add a comment mentioning that "search" does not look inside inline namespaces?
This seems non-obvious.

269

Also due to different parameter types (T vs B)?
So the description is incomplete or am I missing something?

275

NIT: add as the non-template operator != does not correspond to template operator ==

300

NIT: maybe file a bug for this and mention the GH issue number?
(could be done in the last iteration right before landing the change)

usaxena95 updated this revision to Diff 465393.Oct 5 2022, 7:39 AM
usaxena95 marked 11 inline comments as done.

Addressed comments.

usaxena95 added inline comments.Oct 5 2022, 8:38 AM
clang/include/clang/Sema/Overload.h
1033–1035

Just to be clear, it is possible to not reverse the args even if this returns true now since we do not do full check for == here.

I renamed it allowsReversed on the same line of AllowRewrittenCandidates.

Intention is to convey that if this is true then reversing is allowed and you can choose not to do so as well.

clang/lib/Sema/SemaOverload.cpp
975–977

I would prefer not to use a FirstOperand for shouldAddReversed as it serves more than just operator==.

It might be confusing/error-prone for the users as to what is the "first operand" here (Args[0] or Args[1] ? In reversed args or original args ?). I think it would be a better API to just have original args as the parameter let this function decide which is the FirstOperand.

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
231

Yes. That is correct.

269

This verifies that adding const would solve the ambiguity without adding a matching !=. The != here does not match because it is non-template and, yes, because of different param types.

300

Sure. I can do this in a followup patch once this issue lands.

ilya-biryukov accepted this revision.Oct 6 2022, 2:48 AM

LGTM with a few minor NITs

clang/include/clang/Sema/Overload.h
1033–1035

The new name LG, thanks.

clang/lib/Sema/SemaOverload.cpp
975–977

It might be confusing/error-prone for the users as to what is the "first operand" here (Args[0] or Args[1] ?

Yeah, good point. It's much better to have this choice in this function to avoid the complicated contract on each call site.
That being said...

let this function decide which is the FirstOperand.

We should have a comment explaining why *first* operand is Args[1]. It's a bit non-obvious, but followed from the specification.

I also suggest to add an extra sanity check: assert(OriginalArgs.size() == 2).

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
269

However, the rewritten operator does get added in that case, right?
So my reading is that the 'rewrite' does happen, but since conversions are the same, the
(over.match.best.genera)p2.8 kick in and there is no ambiguity:

Given these definitions, a viable function F1 is defined to be a better function than another viable function F2 ...

  • F2 is a rewritten candidate ([over.match.oper]) and F1 is not

I suggest to:

  • remove the operator != completely, we check having it does not block adding rewritten operators in other tests,
  • change the comment to say No ambiguity due to const as the rewrite actually does happen.
This revision is now accepted and ready to land.Oct 6 2022, 2:48 AM
usaxena95 updated this revision to Diff 465686.Oct 6 2022, 3:23 AM
usaxena95 marked 2 inline comments as done.

Addressed comments.

usaxena95 edited the summary of this revision. (Show Details)Oct 6 2022, 4:21 AM
This revision was landed with ongoing or failed builds.Oct 6 2022, 4:22 AM
This revision was automatically updated to reflect the committed changes.

Should cxx_status.html be updated as well? It's listed there as a C++2b paper.

sberg added a subscriber: sberg.Oct 9 2022, 7:30 AM

I just ran into newly-failing

$ cat test.cc
struct S1 {
    bool operator==(int);
    bool operator!=(int);
};
struct S2;
bool operator ==(S2, S2);
bool f(S1 s) { return 0 == s; }
$ clang++ -std=c++20 -fsyntax-only test.cc
test.cc:7:25: error: invalid operands to binary expression ('int' and 'S1')
bool f(S1 s) { return 0 == s; }
                      ~ ^  ~
test.cc:6:6: note: candidate function not viable: no known conversion from 'int' to 'S2' for 1st argument
bool operator ==(S2, S2);
     ^
1 error generated.

It looks to me like this is correctly rejected now per P2468R2. But it is rather confusing that a note mentions the S2 candidate, while the relevant S1 operators == and != are not mentioned at all.

I just ran into newly-failing
It looks to me like this is correctly rejected now per P2468R2.

Yes, this is intentional, the behavior of the code is the same as in C++17.

But it is rather confusing that a note mentions the S2 candidate, while the relevant S1 operators == and != are not mentioned at all.

The diagnostic is indeed not very helpful and it would be nice to improve it.
However, this is unrelated to this change, e.g. it reproduces in C++17 mode with Clang 10.

Should cxx_status.html be updated as well? It's listed there as a C++2b paper.

@usaxena95 we have forgotten to do this indeed. Could you please update the relevant html file?

Sent out https://reviews.llvm.org/D135608 to update implementation status.

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

This is the intended behaviour, yes. The paper aims to revert back to C++17 behavior when we see a matching operator != and this was an error in C++17: https://godbolt.org/z/K3cr9MMh5.
As mentioned above, the actual compiler error message is not very helpful and is something that could be improved, but this was not introduced by this patch.

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

This is the intended behaviour, yes. The paper aims to revert back to C++17 behavior when we see a matching operator != and this was an error in C++17: https://godbolt.org/z/K3cr9MMh5.
As mentioned above, the actual compiler error message is not very helpful and is something that could be improved, but this was not introduced by this patch.

Thank you very much for clarifying! I wasn't sure, and didn't want to mislead @BertalanD.

usaxena95 added a comment.EditedOct 12 2022, 9:43 AM

And Ilya beat me to it.

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

This is intentional. Example from https://eel.is/c++draft/over.match.oper#4

struct A {};
template<typename T> bool operator==(A, T);     // #1
bool a1 = 0 == A();                             // OK, calls reversed #1
template<typename T> bool operator!=(A, T);
bool a2 = 0 == A();                             // error, #1 is not a rewrite target

Adding a matching operator!= disables reversing arguments (C++17 mode). The LHS in this case should provide a operator==.

The diagnostic might not be the most helpful at this point is. You can choose to

  • Remove operator!=. Allows assuming x==y is equivalent to y==x and therefore uses the same function with reversed arguments.
  • Write s == c instead of c == s.

Thank you all for the help and clarification! I'll go fix our code then.

As an aside, I understand that this paper now retroactively applies to the C++ standard, but code has already been written that assumes that what my example is doing is valid. Does it deserve a mention in the Potentially Breaking Changes section of the release notes?

Does it deserve a mention in the Potentially Breaking Changes section of the release notes?

You are right. I was thinking the same after this discussion. I will add this to the section.