[clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.

[massberg]

Summary

This patch implemed the change proposed in [P2002R1] to 11.11.1 [class.compare.default] paragraph 1.

A defaulted compariosn operator function must be non-volatile and must either have no ref-qualifier or the ref-qualifier &.

[ilya-biryukov]

Thanks! I believe we should also recover in the similar manner we do for missing const, see corresponding comment.

Extra NITs:

• there is a typo in description: *compariosn* should be comparison,
• we probably want to add this change to release notes.

Also adding @clang-language-wg in case someone else wants to chime in.

clang/include/clang/Basic/DiagnosticSemaKinds.td

9434

NIT: for consistency with wording of notes above.

9436

NIT: for consistency with the wording of err_ref_qualifier_constructor

clang/lib/Sema/SemaDeclCXX.cpp

8593–8601

NIT: this version is simpler and more aligned with the code below.
Also, could you move the volatile handling after the check for const?

Additionally, we seem to recover in case of const by adding it to the type and suggesting a fix-it to add it in the code.
Could we do the same for volatile and ref-qualifier, i.e. suggest a fix-it to remove the ref-qualifier and volatile and change the corresponding type to make AST pretend they were never there?

[shafik]

clang/test/CXX/class/class.compare/class.compare.default/p1.cpp

18

Can we also have examples with out of line defaulting.

[aaron.ballman]

Precommit CI found a relevant failure that should be addressed.

clang/lib/Sema/SemaDeclCXX.cpp

8593

8593–8601

Note, you can shorten it further with:

return Diag(...);

because that will return true for you.

[massberg]

Thanks for the comments!
Updated error messages, code and tests.

[massberg]

clang/lib/Sema/SemaDeclCXX.cpp

8593

Thanks! Which the change suggested by Ilya this has become obsolete.

8593–8601

Thanks, this is much simpler!

I have moved the code and added a recovery. However, adding a fix-it is much more complex as there is no simple way to get location to remove the keywords. Thus I decided to not offering a fix-it in this patch.

[aaron.ballman]

Basically looking good to me now, but the changes need a release note and I does this mean we can change cxx_status.html to claim full support for P2002R1 now or is there more left to be done for that? (If there's more left to do, adding those details to the cxx_status page would be appropriate.)

clang/include/clang/Basic/DiagnosticSemaKinds.td

9436

Still need to drop the s from operators

clang/lib/Sema/SemaDeclCXX.cpp

8593–8601

Yeah, it seems we don't track the ref qualifier location on any of the CXXMethodDecl, FunctionProtoType, or FunctionProtoTypeLoc classes. A future refactoring can consider adding locations for those to FunctionProtoTypeLoc, but needs to be able to handle multiple qualifiers (e.g., void func() const &; should track both the const and the & separately).

[massberg]

Fixed typo, added release note and updated cxx_status.html.

[aaron.ballman]

Did you verify that the rest of P2002R1 was implemented correctly? There are a lot more changes in that paper than just this tiny bit, so if we're not certain this finishes things, we shouldn't mark the status as complete (but if we're figuring out what's missing, we should capture those details somewhere so we know what's left to do)

https://github.com/llvm/llvm-project/commit/b74a381296eef048911bb22dc4eb2d3598460470 is what marked the status as partial, but there's no explanation as to why it was partial. Perhaps @rsmith remembers what was left to be done there.

[massberg]

Fix test that broke after last change.

[massberg]

I have checked the paper P2002R1 and as far as I can tell it is fully implemented when this patch has landed.

[shafik]

In D148924#4318381, @massberg wrote:

I have checked the paper P2002R1 and as far as I can tell it is fully implemented when this patch has landed.

So I read the paper but since the paper does not have examples for each change nor does it have any links to DRs it is not obvious without spending some time to verify that we support each section. Changes like these really should come with examples in the paper to demonstrate behavior before and after.

I searched in the code and found that @rsmith labelled P2002R0 changes with comments and so a cursory look at those changes side by side with the paper seems to indicate that we probably covered most of the cases but I am not 100% sure. There is test coverage added with those changes as well but again relating each test to specific paper section is not trivial, at least for me.

The paper also uses the term constexpr compatible which has been replaced with constexpr suitable and not sure if that has any practical effect here.

[ilya-biryukov]

In D148924#4320694, @shafik wrote:

The paper also uses the term constexpr compatible which has been replaced with constexpr suitable and not sure if that has any practical effect here.

They seem to be different terms, constexpr-suitable does not actually replace constexpr-compatible. The former refers to more basic checks (i.e. "not a coroutine", "not a ctor or dtor for a class with virtual bases"), the latter only pops up in context of defaulted functions.
Constexpr-compatible has been removed completely in P2448r2, but I think it's only applicable in C++23 and this change aims to implement C++20.

[ilya-biryukov]

@massberg did we figure out if there is anything else left from P2002R1? Are there blockers to landing this?

[massberg]

In D148924#4391190, @ilya-biryukov wrote:

@massberg did we figure out if there is anything else left from P2002R1? Are there blockers to landing this?

The only thing I came across was https://reviews.llvm.org/D151200 (error out when parameters of defaulted comparison operator aren't as expected), but this is already submitted.
Related to the defaulted comparison operators is DR 2450 (braced-init-list as a template-argument), but this isn't part of P2002R1.

So I'm not aware of any blockers.

[ilya-biryukov]

Since there are no remaining blockers, LGTM from my side.
I suggest waiting for ~day to give chance to other reviewers.

@shafik, @aaron.ballman, @rsmith please let us know if you want to make another run through the paper to do extra checking.

[aaron.ballman]

LGTM aside from a minor issue with the status page (we use unreleased until the current version is released).

clang/www/cxx_status.html

1008 ↗

(On Diff #519432)

[shafik]

LGTM

[massberg]

Fix status of paper in status page to unreleased.

[massberg]

Format code.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_defaulted_comparison_not_constexpr : Note<
   "non-constexpr comparison function would be used to compare "
   "%select{|member %1|base class %1}0">;
 def note_defaulted_comparison_not_constexpr_here : Note<
   "non-constexpr comparison function declared here">;
 def note_in_declaration_of_implicit_equality_comparison : Note<
   "while declaring the corresponding implicit 'operator==' "
   "for this defaulted 'operator<=>'">;
+def err_volatile_comparison_operator : Error<
+  "defaulted comparison operator function must not be volatile">;
+def err_ref_qualifier_comparison_operator : Error<
+  "ref-qualifier '&&' is not allowed on defaulted comparison operators">;
 
 def ext_implicit_exception_spec_mismatch : ExtWarn<
   "function previously declared with an %select{explicit|implicit}0 exception "
   "specification redeclared with an %select{implicit|explicit}0 exception "
   "specification">, InGroup<DiagGroup<"implicit-exception-spec-mismatch">>;
 
 def warn_ptr_arith_precedes_bounds : Warning<
   "the pointer decremented by %0 refers before the beginning of the array">,

clang/lib/Sema/SemaDeclCXX.cpp

lookupOperatorsForDefaultedComparison(*this, S, Operators,
                                           FD->getOverloadedOperator());
     FD->setDefaultedFunctionInfo(FunctionDecl::DefaultedFunctionInfo::Create(
         Context, Operators.pairs()));
   }
 
   // C++2a [class.compare.default]p1:
   //   A defaulted comparison operator function for some class C shall be a
   //   non-template function declared in the member-specification of C that is
-  //    -- a non-static const member of C having one parameter of type
-  //       const C&, or
+  //    -- a non-static const non-volatile member of C having one parameter of type
+  //       const C& and either no ref-qualifier or the ref-qualifier &, or
   //    -- a friend of C having two parameters of type const C& or two
   //       parameters of type C.
 
   CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(FD->getLexicalDeclContext());
   bool IsMethod = isa<CXXMethodDecl>(FD);
   if (IsMethod) {
     auto *MD = cast<CXXMethodDecl>(FD);
     assert(!MD->isStatic() && "comparison function cannot be a static member");
 
+    const FunctionProtoType *FnType = FD->getType()->castAs<FunctionProtoType>();
+    if (FnType->isVolatile()) {
+      Diag(FD->getLocation(), diag::err_volatile_comparison_operator);
+      return true;
+    }
+    if (FnType->getRefQualifier() == RQ_RValue) {
+      Diag(FD->getLocation(), diag::err_ref_qualifier_comparison_operator);
+      return true;
+    }
+
     // If we're out-of-class, this is the class we're comparing.
     if (!RD)
       RD = MD->getParent();
 
     if (!MD->isConst()) {
       SourceLocation InsertLoc;
       if (FunctionTypeLoc Loc = MD->getFunctionTypeLoc())
         InsertLoc = getLocForEndOfToken(Loc.getRParenLoc());

clang/test/CXX/class/class.compare/class.compare.default/p1.cpp

struct A {
   friend bool operator!=(const A&, const B&) = default; // expected-error {{parameters for defaulted equality comparison operator must have the same type (found 'const A &' vs 'const B &')}}
   friend bool operator!=(const B&, const B&) = default; // expected-error {{invalid parameter type for defaulted equality comparison}}
   friend bool operator<(const A&, const A&);
   friend bool operator<(const B&, const B&) = default; // expected-error {{invalid parameter type for defaulted relational comparison}}
   friend bool operator>(A, A) = default; // expected-warning {{implicitly deleted}} expected-note{{replace 'default'}}
 
   bool operator<(const A&) const;
   bool operator<=(const A&) const = default;
-  bool operator==(const A&) const volatile && = default; // surprisingly, OK
+  bool operator==(const A&) const && = default; // expected-error {{ref-qualifier '&&' is not allowed on defaulted comparison operators}}
+  bool operator>=(const A&) const volatile = default; // expected-error {{defaulted comparison operator function must not be volatile}}
   bool operator<=>(const A&) = default; // expected-error {{defaulted member three-way comparison operator must be const-qualified}}
   bool operator>=(const B&) const = default; // expected-error-re {{invalid parameter type for defaulted relational comparison operator; found 'const B &', expected 'const A &'{{$}}}}
   static bool operator>(const B&) = default; // expected-error {{overloaded 'operator>' cannot be a static member function}}
   friend bool operator>(A, const A&) = default; // expected-error {{must have the same type}} expected-note {{would be the best match}}
 
   template<typename T = void>
     friend bool operator==(const A&, const A&) = default; // expected-error {{comparison operator template cannot be defaulted}}
   template<typename T = void>