[clang] p2085 out-of-class comparison operator defaulting

[urnathan]

Summary

This implements p2085, allowing out-of-class defaulting of comparison operators, primarily so they need not be inline, IIUC intent. this was mostly straigh forward, but required reimplementing Sema::CheckExplicitlyDefaultedComparison, as now there's a case where we have no a priori clue as to what class a defaulted comparison may be for. We have to inspect the parameter types to find out. Eg:

class X { ... };
bool operator==(X, X) = default;

Thus reimplemented the parameter type checking, and added 'is this a friend' functionality for the above case.

AFAICT, this is affecting code Richard authored.

[urnathan]

ping?

[urnathan]

updated against current HEAD. ping?

[urnathan]

ping?

[urnathan]

updated to trunk. ping?

[urnathan]

Correct the cxx_status clang version number. Let's not let this roll over again!

[mizvekov]

Hello! Thanks for this, just a few small comments, I'll try to give it a more careful look later!

clang/lib/Sema/SemaDeclCXX.cpp

8437

The curly braces are not needed.

8452–8455

Our style guidelines dictate that the braces are all or nothing :)

8468

Our style says to construct with curlies only simple aggregate classes.

8504–8514

If you would like to make this a bit shorter.

[urnathan]

This comment was removed by urnathan.

[urnathan]

Let's try that again. Thanks for the formatting guide and pointing at find_if. I think this addresses all those points.

[urnathan]

clang/lib/Sema/SemaDeclCXX.cpp

8452–8455

FWIW, this doesnt seem adhered to very consistently. I did not deduce that from the existing code.

[mizvekov]

clang/lib/Sema/SemaDeclCXX.cpp

8431–8432

I think this might not be correct and we will miss diagnosing a case like:

template <class T> struct A {
  bool operator==(T const &) const;
};
template <class T> bool A<T>::operator==(T const &) const = default;
template struct A<int>;

8436

The whole logic involving this Ok is a little bit hard to follow, but I see that it's not straightforward to simplify.
The main thing here I think is that we are checking many different things and so it's hard to give it a less generic name.
Just a note on my part though, I don't have specific suggestions :)

8468–8469

Looking at this more carefully, we did not even need to set PlainTy to null in the first place, in that IsMethod==true case it will just be ignored in the diagnostics.

[mizvekov]

clang/lib/Sema/SemaDeclCXX.cpp

8431–8432

So I just downloaded the patch to try this out, there are indeed problems.

This test case is accepted:

template <class T> struct S6 {
  bool operator==(T const &) const;
};
template <class T> bool S6<T>::operator==(T const &) const = default;

// template struct S6<int>;

void f() {
  S6<int> a;
  (void)(a == 0);
}

If you uncomment the line, it crashes clang instead, with this backtrace:

PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: llvm\\bin\\clang.exe -cc1 -internal-isystem llvm\\lib\\clang\\14.0.0\\include -nostdsysteminc -std=c++2a -verify clang\\test\\CXX\\class\\class.compare\\class.compare.default\\p1.cpp
1.      clang\test\CXX\class\class.compare\class.compare.default\p1.cpp:174:16: current parser token ')'
2.      clang\test\CXX\class\class.compare\class.compare.default\p1.cpp:129:1: parsing namespace 'p2085'
3.      clang\test\CXX\class\class.compare\class.compare.default\p1.cpp:172:10: parsing function body 'p2085::f'
4.      clang\test\CXX\class\class.compare\class.compare.default\p1.cpp:172:10: in compound statement ('{}')
 #0 0x00007ff7c6c86dcb clang::Redeclarable<class clang::TagDecl>::getFirstDecl(void) clang\include\clang\AST\Redeclarable.h:216:0
 #1 0x00007ff7c6c8a7b4 clang::Redeclarable<class clang::TagDecl>::getMostRecentDecl(void) clang\include\clang\AST\Redeclarable.h:227:0
 #2 0x00007ff7c6c8a89b clang::RecordDecl::getMostRecentDecl(void) clang\include\clang\AST\Decl.h:3907:0
 #3 0x00007ff7c6c8a814 clang::CXXRecordDecl::getMostRecentDecl(void) clang\include\clang\AST\DeclCXX.h:512:0
 #4 0x00007ff7c6c8a844 clang::CXXRecordDecl::getMostRecentDecl(void) const clang\include\clang\AST\DeclCXX.h:518:0
 #5 0x00007ff7c6c57d74 clang::CXXRecordDecl::dataPtr(void) const clang\include\clang\AST\DeclCXX.h:430:0
 #6 0x00007ff7c6c57c84 clang::CXXRecordDecl::data(void) const clang\include\clang\AST\DeclCXX.h:434:0
 #7 0x00007ff7c6c4cfb4 clang::CXXRecordDecl::bases_begin(void) clang\include\clang\AST\DeclCXX.h:596:0
 #8 0x00007ff7c6c4cfe4 clang::CXXRecordDecl::bases_end(void) clang\include\clang\AST\DeclCXX.h:598:0
 #9 0x00007ff7c6c4cf59 clang::CXXRecordDecl::bases(void) clang\include\clang\AST\DeclCXX.h:590:0
#10 0x00007ff7cb0e31a8 `anonymous namespace'::DefaultedComparisonVisitor<`anonymous namespace'::DefaultedComparisonSynthesizer,A0xf8d43d5f::StmtListResult,clang::ActionResult<clang::Stmt *,1>,std::pair<clang::ActionResult<clang::Expr *,1>,clang::ActionResult<clang::Expr *,1> > >::visitSubobjects clang\lib\Sema\SemaDeclCXX.cpp:7626:0
#11 0x00007ff7cb0e302e `anonymous namespace'::DefaultedComparisonVisitor<`anonymous namespace'::DefaultedComparisonSynthesizer,A0xf8d43d5f::StmtListResult,clang::ActionResult<clang::Stmt *,1>,std::pair<clang::ActionResult<clang::Expr *,1>,clang::ActionResult<clang::Expr *,1> > >::visit clang\lib\Sema\SemaDeclCXX.cpp:7604:0
#12 0x00007ff7cb0e3738 `anonymous namespace'::DefaultedComparisonSynthesizer::build clang\lib\Sema\SemaDeclCXX.cpp:8061:0
#13 0x00007ff7cb0bc39c clang::Sema::DefineDefaultedComparison(class clang::SourceLocation, class clang::FunctionDecl *, enum clang::Sema::DefaultedComparisonKind) clang\lib\Sema\SemaDeclCXX.cpp:8739:0
#14 0x00007ff7cb933160 <lambda_1975556f3529fa94ad6319a5b3e731e2>::operator() clang\lib\Sema\SemaExpr.cpp:17223:0
#15 0x00007ff7cb9a4ed4 llvm::function_ref<void __cdecl(void)>::callback_fn<<lambda_1975556f3529fa94ad6319a5b3e731e2> > llvm\include\llvm\ADT\STLExtras.h:179:0
#16 0x00007ff7c655009a llvm::function_ref<(void)>::operator()(void) const llvm\include\llvm\ADT\STLExtras.h:201:0
#17 0x00007ff7c3fc207f clang::runWithSufficientStackSpace(class llvm::function_ref<(void)>, class llvm::function_ref<(void)>) clang\include\clang\Basic\Stack.h:52:0
#18 0x00007ff7cae65393 clang::Sema::runWithSufficientStackSpace(class clang::SourceLocation, class llvm::function_ref<(void)>) clang\lib\Sema\Sema.cpp:489:0
#19 0x00007ff7cb8b0b38 clang::Sema::MarkFunctionReferenced(class clang::SourceLocation, class clang::FunctionDecl *, bool) clang\lib\Sema\SemaExpr.cpp:17281:0
#20 0x00007ff7cb8b079d clang::Sema::MarkAnyDeclReferenced(class clang::SourceLocation, class clang::Decl *, bool) clang\lib\Sema\SemaExpr.cpp:18819:0
#21 0x00007ff7cb937e39 MarkExprReferenced clang\lib\Sema\SemaExpr.cpp:18736:0
#22 0x00007ff7cb8b10e3 clang::Sema::MarkDeclRefReferenced(class clang::DeclRefExpr *, class clang::Expr const *) clang\lib\Sema\SemaExpr.cpp:18776:0
#23 0x00007ff7cc0bd380 CreateFunctionRefExpr clang\lib\Sema\SemaOverload.cpp:73:0
#24 0x00007ff7cc0b2cec clang::Sema::CreateOverloadedBinOp(class clang::SourceLocation, enum clang::BinaryOperatorKind, class clang::UnresolvedSetImpl const &, class clang::Expr *, class clang::Expr *, bool, bool, class clang::FunctionDecl *) clang\lib\Sema\SemaOverload.cpp:13759:0
#25 0x00007ff7cb90b61f BuildOverloadedBinOp clang\lib\Sema\SemaExpr.cpp:14645:0
#26 0x00007ff7cb8d1a0f clang::Sema::BuildBinOp(class clang::Scope *, class clang::SourceLocation, enum clang::BinaryOperatorKind, class clang::Expr *, class clang::Expr *) clang\lib\Sema\SemaExpr.cpp:14747:0
#27 0x00007ff7cb8d1093 clang::Sema::ActOnBinOp(class clang::Scope *, class clang::SourceLocation, enum clang::tok::TokenKind, class clang::Expr *, class clang::Expr *) clang\lib\Sema\SemaExpr.cpp:14604:0
#28 0x00007ff7cada2e98 clang::Parser::ParseRHSOfBinaryExpression(class clang::ActionResult<class clang::Expr *, 1>, enum clang::prec::Level) clang\lib\Parse\ParseExpr.cpp:631:0
#29 0x00007ff7cada155a clang::Parser::ParseAssignmentExpression(enum clang::Parser::TypeCastState) clang\lib\Parse\ParseExpr.cpp:176:0
#30 0x00007ff7cadae36e clang::Parser::ParseSimpleExpressionList(class llvm::SmallVectorImpl<class clang::Expr *> &, class llvm::SmallVectorImpl<class clang::SourceLocation> &) clang\lib\Parse\ParseExpr.cpp:3422:0
#31 0x00007ff7cadafd37 clang::Parser::ParseParenExpression(enum clang::Parser::ParenParseOption &, bool, bool, class clang::OpaquePtr<class clang::QualType> &, class clang::SourceLocation &) clang\lib\Parse\ParseExpr.cpp:3058:0
#32 0x00007ff7cada36e3 clang::Parser::ParseCastExpression(enum clang::Parser::CastParseKind, bool, bool &, enum clang::Parser::TypeCastState, bool, bool *) clang\lib\Parse\ParseExpr.cpp:957:0
#33 0x00007ff7cada8b26 clang::Parser::ParseCastExpression(enum clang::Parser::CastParseKind, bool, enum clang::Parser::TypeCastState, bool, bool *) clang\lib\Parse\ParseExpr.cpp:687:0
#34 0x00007ff7cadafa77 clang::Parser::ParseParenExpression(enum clang::Parser::ParenParseOption &, bool, bool, class clang::OpaquePtr<class clang::QualType> &, class clang::SourceLocation &) clang\lib\Parse\ParseExpr.cpp:3033:0
#35 0x00007ff7cada36e3 clang::Parser::ParseCastExpression(enum clang::Parser::CastParseKind, bool, bool &, enum clang::Parser::TypeCastState, bool, bool *) clang\lib\Parse\ParseExpr.cpp:957:0
#36 0x00007ff7cada8b26 clang::Parser::ParseCastExpression(enum clang::Parser::CastParseKind, bool, enum clang::Parser::TypeCastState, bool, bool *) clang\lib\Parse\ParseExpr.cpp:687:0
#37 0x00007ff7cada153a clang::Parser::ParseAssignmentExpression(enum clang::Parser::TypeCastState) clang\lib\Parse\ParseExpr.cpp:176:0
#38 0x00007ff7cada06ee clang::Parser::ParseExpression(enum clang::Parser::TypeCastState) clang\lib\Parse\ParseExpr.cpp:125:0
#39 0x00007ff7caddca7f clang::Parser::ParseExprStatement(enum clang::Parser::ParsedStmtContext) clang\lib\Parse\ParseStmt.cpp:465:0
#40 0x00007ff7caddb8e8 clang::Parser::ParseStatementOrDeclarationAfterAttributes(class llvm::SmallVector<class clang::Stmt *, 32> &, enum clang::Parser::ParsedStmtContext, class clang::SourceLocation *, struct clang::ParsedAttributesWithRange &) clang\lib\Parse\ParseStmt.cpp:243:0
#41 0x00007ff7caddb1a8 clang::Parser::ParseStatementOrDeclaration(class llvm::SmallVector<class clang::Stmt *, 32> &, enum clang::Parser::ParsedStmtContext, class clang::SourceLocation *) clang\lib\Parse\ParseStmt.cpp:115:0
#42 0x00007ff7caddeb49 clang::Parser::ParseCompoundStatementBody(bool) clang\lib\Parse\ParseStmt.cpp:1111:0
#43 0x00007ff7cade55b2 clang::Parser::ParseFunctionStatementBody(class clang::Decl *, class clang::Parser::ParseScope &) clang\lib\Parse\ParseStmt.cpp:2346:0
#44 0x00007ff7caceec07 clang::Parser::ParseFunctionDefinition(class clang::ParsingDeclarator &, struct clang::Parser::ParsedTemplateInfo const &, class clang::Parser::LateParsedAttrList *) clang\lib\Parse\Parser.cpp:1378:0
#45 0x00007ff7cad33d02 clang::Parser::ParseDeclGroup(class clang::ParsingDeclSpec &, enum clang::DeclaratorContext, class clang::SourceLocation *, struct clang::Parser::ForRangeInit *) clang\lib\Parse\ParseDecl.cpp:2041:0
#46 0x00007ff7caced8f7 clang::Parser::ParseDeclOrFunctionDefInternal(struct clang::ParsedAttributesWithRange &, class clang::ParsingDeclSpec &, enum clang::AccessSpecifier) clang\lib\Parse\Parser.cpp:1138:0
#47 0x00007ff7caced1a2 clang::Parser::ParseDeclarationOrFunctionDefinition(struct clang::ParsedAttributesWithRange &, class clang::ParsingDeclSpec *, enum clang::AccessSpecifier) clang\lib\Parse\Parser.cpp:1154:0
#48 0x00007ff7cacecbb0 clang::Parser::ParseExternalDeclaration(struct clang::ParsedAttributesWithRange &, class clang::ParsingDeclSpec *) clang\lib\Parse\Parser.cpp:976:0
#49 0x00007ff7cadf9278 clang::Parser::ParseInnerNamespace(class llvm::SmallVector<struct clang::Parser::InnerNamespaceInfo, 4> const &, unsigned int, class clang::SourceLocation &, class clang::ParsedAttributes &, class clang::BalancedDelimiterTracker &) clang\lib\Parse\ParseDeclCXX.cpp:247:0
#50 0x00007ff7cadf9060 clang::Parser::ParseNamespace(enum clang::DeclaratorContext, class clang::SourceLocation &, class clang::SourceLocation) clang\lib\Parse\ParseDeclCXX.cpp:227:0
#51 0x00007ff7cad32d4a clang::Parser::ParseDeclaration(enum clang::DeclaratorContext, class clang::SourceLocation &, struct clang::ParsedAttributesWithRange &, class clang::SourceLocation *) clang\lib\Parse\ParseDecl.cpp:1742:0
#52 0x00007ff7cacec629 clang::Parser::ParseExternalDeclaration(struct clang::ParsedAttributesWithRange &, class clang::ParsingDeclSpec *) clang\lib\Parse\Parser.cpp:910:0
#53 0x00007ff7cace7384 clang::Parser::ParseTopLevelDecl(class clang::OpaquePtr<class clang::DeclGroupRef> &, bool) clang\lib\Parse\Parser.cpp:720:0
#54 0x00007ff7cace36a8 clang::ParseAST(class clang::Sema &, bool, bool) clang\lib\Parse\ParseAST.cpp:158:0
#55 0x00007ff7c7b7e1e7 clang::ASTFrontendAction::ExecuteAction(void) clang\lib\Frontend\FrontendAction.cpp:1066:0
#56 0x00007ff7c7b7db9e clang::FrontendAction::Execute(void) clang\lib\Frontend\FrontendAction.cpp:961:0
#57 0x00007ff7c7afbb96 clang::CompilerInstance::ExecuteAction(class clang::FrontendAction &) clang\lib\Frontend\CompilerInstance.cpp:1027:0
#58 0x00007ff7c7d68ee8 clang::ExecuteCompilerInvocation(class clang::CompilerInstance *) clang\lib\FrontendTool\ExecuteCompilerInvocation.cpp:261:0
#59 0x00007ff7c3d8e6b4 cc1_main(class llvm::ArrayRef<char const *>, char const *, void *) clang\tools\driver\cc1_main.cpp:246:0

[urnathan]

thanks for the testcase. The problem is not that we're not checking dependent types at that point, but that we don't check later when we have an instantiation. the 'evil' testcase shows why one cannot check earlier -- though I hope no one writes that! The instantiation code was 'optimized' with the knowledge that comparison operators can only be defaulted in-class, which is of course no longer true with p2085.

Am I correct in thinking that instantiating a function definition doesn't create a new FunctionDecl, but attaches the definition to the already-instantiated FunctionDecl instance? See the code where I update the instantiation's location in the out-of-class defaulting case. Without that change, the errors for bad default parameter types point at the in-class declaration, which is not helpful.

[urnathan]

remove unneeded PlainTy clearing

[mizvekov]

In D104478#3093020, @urnathan wrote:

Am I correct in thinking that instantiating a function definition doesn't create a new FunctionDecl, but attaches the definition to the already-instantiated FunctionDecl instance? See the code where I update the instantiation's location in the out-of-class defaulting case. Without that change, the errors for bad default parameter types point at the in-class declaration, which is not helpful.

If you look at the AST dump I posted in my other comment, you have the comparison operator's CXXMethodDecl associated to the TemplateDecl, and another one associated to the TemplateSpecializationDecl.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

5003–5006 ↗

(On Diff #384766)

You can check the result of this using clang's ast-dump:

Source:

template <class T> struct S6 {
  bool operator==(T const &) const;
};
template <class T> bool S6<T>::operator==(T const &) const = default;
template struct S6<int>;

clang++ -cc1 -fsyntax-only -std=c++20 -ast-dump

AST produced with this change:

|-ClassTemplateDecl 0x150a4970e88 <..\..\..\clang-snipets\D104478\test1.cc:1:1, line:3:1> line:1:27 S6
| |-TemplateTypeParmDecl 0x150a49016b0 <col:11, col:17> col:17 referenced class depth 0 index 0 T
| |-CXXRecordDecl 0x150a4901778 <col:20, line:3:1> line:1:27 struct S6 definition
| | |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial constexpr needs_implicit
| | |-CXXRecordDecl 0x150a49710e8 <col:20, col:27> col:27 implicit struct S6
| | `-CXXMethodDecl 0x150a4971340 <line:2:3, col:30> col:8 operator== 'bool (const T &) const'
| |   `-ParmVarDecl 0x150a4971200 <col:19, col:27> col:28 'const T &'
| `-ClassTemplateSpecialization 0x150a4971810 'S6'
|-CXXMethodDecl 0x150a4971700 parent 0x150a4901778 prev 0x150a4971340 <line:4:1, col:54> col:32 operator== 'bool (const T &) const' default
| `-ParmVarDecl 0x150a4971620 <col:43, col:51> col:52 'const T &'
`-ClassTemplateSpecializationDecl 0x150a4971810 <line:5:1, col:23> col:17 struct S6 definition
  |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial constexpr needs_implicit
  |-TemplateArgument type 'int'
  | `-BuiltinType 0x150a4900950 'int'
  |-CXXRecordDecl 0x150a4971a10 prev 0x150a4971810 <line:1:20, col:27> col:27 implicit struct S6
  `-CXXMethodDecl 0x150a4971c40 <line:4:20, line:2:30> line:4:32 invalid operator== 'bool (const int &) const' default
    `-ParmVarDecl 0x150a4971b40 <line:2:19, col:27> col:28 'const int &'

Second to last line, you have the CXXMethodDecl with the adjusted location, but it's ParmVarDecl's location still refers to the one in the original declaration.

I don't think you are supposed to do this kind of adjustment, it's more likely there is a bug / missing change somewhere else.

[urnathan]

When we instantiate a template definition, we don't instantiate a new declaration. That's fine when the introducing declaration is the definition. But when we have a forward declaration, we can run into problems. For instance, regard:

template<typename T> struct S {
  void frob (T const &a);
};

template<typename T> void S<T>::frob (T const &bad)
{ bad = 5; }

template struct S<int>;

devvm3900:55>build/bin/clang++ -c mem.ii

mem.ii:6:7: error: cannot assign to variable 'bad' with const-qualified type 'const int &'
{ bad = 5; }
  ~~~ ^
mem.ii:8:17: note: in instantiation of member function 'S<int>::frob' requested here
template struct S<int>;
                ^
mem.ii:2:23: note: variable 'bad' declared const here
  void frob (T const &a);
             ~~~~~~~~~^

notice the location of where it claims 'bad' is!

So let's just avoid working around existing issue with template definitions not being the introducing decl, and leave that as an orthogonal problem.

[mizvekov]

From my POV this looks good.

I would have perhaps added some AST tests here.

ping @rsmith any thoughts / suggestions / improvements here?

[mizvekov]

In D104478#3131758, @urnathan wrote:

When we instantiate a template definition, we don't instantiate a new declaration. That's fine when the introducing declaration is the definition. But when we have a forward declaration, we can run into problems. For instance, regard:

I think the -ast-dump of that example shows that we do instantiate a new declaration:

|-ClassTemplateDecl 0x1a28478a968 <test.cc:1:1, line:3:1> line:1:29 S
| |-TemplateTypeParmDecl 0x1a28471af50 <col:10, col:19> col:19 referenced typename depth 0 index 0 T
| |-CXXRecordDecl 0x1a28471b018 <col:22, line:3:1> line:1:29 struct S definition
| | |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial constexpr needs_implicit
| | |-CXXRecordDecl 0x1a28478abc8 <col:22, col:29> col:29 implicit struct S
| | `-CXXMethodDecl 0x1a28478ae20 <line:2:3, col:24> col:8 frob 'void (const T &)' <=== #1
| |   `-ParmVarDecl 0x1a28478ace0 <col:14, col:23> col:23 a 'const T &'
| `-ClassTemplateSpecialization 0x1a28478b370 'S'
|-CXXMethodDecl 0x1a28478b1e0 parent 0x1a28471b018 prev 0x1a28478ae20 <line:5:1, line:6:12> line:5:33 frob 'void (const T &)' <=== #2
| |-ParmVarDecl 0x1a28478b100 <col:39, col:48> col:48 referenced bad 'const T &'
| `-CompoundStmt 0x1a28478b338 <line:6:1, col:12>
|   `-BinaryOperator 0x1a28478b318 <col:3, col:9> '<dependent type>' '='
|     |-DeclRefExpr 0x1a28478b2d0 <col:3> 'const T' lvalue ParmVar 0x1a28478b100 'bad' 'const T &'
|     `-IntegerLiteral 0x1a28478b2f0 <col:9> 'int' 5

But I agree this is a separate problem that can be solved in another patch.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def select_defaulted_comparison_kind : TextSubstitution<
   "operator">;
 def ext_defaulted_comparison : ExtWarn<
   "defaulted comparison operators are a C++20 extension">, InGroup<CXX20>;
 def warn_cxx17_compat_defaulted_comparison : Warning<
   "defaulted comparison operators are incompatible with C++ standards "
   "before C++20">, InGroup<CXXPre20Compat>, DefaultIgnore;
 def err_defaulted_comparison_template : Error<
   "comparison operator template cannot be defaulted">;
-def err_defaulted_comparison_out_of_class : Error<
-  "%sub{select_defaulted_comparison_kind}0 can only be defaulted in a class "
-  "definition">;
+def err_defaulted_comparison_num_args : Error<
+  "%select{non-member|member}0 %sub{select_defaulted_comparison_kind}1"
+  " comparison operator must have %select{2|1}0 parameters">;
 def err_defaulted_comparison_param : Error<
   "invalid parameter type for defaulted %sub{select_defaulted_comparison_kind}0"
   "; found %1, expected %2%select{| or %4}3">;
+def err_defaulted_comparison_param_unknown : Error<
+  "invalid parameter type for non-member defaulted"
+  " %sub{select_defaulted_comparison_kind}0"
+  "; found %1, expected class or reference to a constant class">;
 def err_defaulted_comparison_param_mismatch : Error<
   "parameters for defaulted %sub{select_defaulted_comparison_kind}0 "
   "must have the same type%diff{ (found $ vs $)|}1,2">;
+def err_defaulted_comparison_not_friend : Error<
+  "%sub{select_defaulted_comparison_kind}0 is not a friend of"
+  " %select{|incomplete class }1%2">;
 def err_defaulted_comparison_non_const : Error<
   "defaulted member %sub{select_defaulted_comparison_kind}0 must be "
   "const-qualified">;
 def err_defaulted_comparison_return_type_not_bool : Error<
   "return type for defaulted %sub{select_defaulted_comparison_kind}0 "
   "must be 'bool', not %1">;
 def err_defaulted_comparison_deduced_return_type_not_auto : Error<
   "deduced return type for defaulted %sub{select_defaulted_comparison_kind}0 "

clang/lib/Sema/SemaDeclCXX.cpp

if (Op == OO_Spaceship) {
     Lookup(OO_EqualEqual);
   }
 }
 
 bool Sema::CheckExplicitlyDefaultedComparison(Scope *S, FunctionDecl *FD,
                                               DefaultedComparisonKind DCK) {
   assert(DCK != DefaultedComparisonKind::None && "not a defaulted comparison");
 
-  CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(FD->getLexicalDeclContext());
-  assert(RD && "defaulted comparison is not defaulted in a class");
-
   // Perform any unqualified lookups we're going to need to default this
   // function.
   if (S) {
     UnresolvedSet<32> Operators;
     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 friend of C having two parameters of type const C& or two
   //       parameters of type C.
-  QualType ExpectedParmType1 = Context.getRecordType(RD);
-  QualType ExpectedParmType2 =
-      Context.getLValueReferenceType(ExpectedParmType1.withConst());
-  if (isa<CXXMethodDecl>(FD))
-    ExpectedParmType1 = ExpectedParmType2;
-  for (const ParmVarDecl *Param : FD->parameters()) {
-    if (!Param->getType()->isDependentType() &&
-        !Context.hasSameType(Param->getType(), ExpectedParmType1) &&
-        !Context.hasSameType(Param->getType(), ExpectedParmType2)) {
-      // Don't diagnose an implicit 'operator=='; we will have diagnosed the
-      // corresponding defaulted 'operator<=>' already.
-      if (!FD->isImplicit()) {
-        Diag(FD->getLocation(), diag::err_defaulted_comparison_param)
-            << (int)DCK << Param->getType() << ExpectedParmType1
-            << !isa<CXXMethodDecl>(FD)
-            << ExpectedParmType2 << Param->getSourceRange();
-      }
-      return true;
-    }
-  }
-  if (FD->getNumParams() == 2 &&
-      !Context.hasSameType(FD->getParamDecl(0)->getType(),
-                           FD->getParamDecl(1)->getType())) {
-    if (!FD->isImplicit()) {
-      Diag(FD->getLocation(), diag::err_defaulted_comparison_param_mismatch)
-          << (int)DCK
-          << FD->getParamDecl(0)->getType()
-          << FD->getParamDecl(0)->getSourceRange()
-          << FD->getParamDecl(1)->getType()
-          << FD->getParamDecl(1)->getSourceRange();
-    }
-    return true;
-  }
 
-  // ... non-static const member ...
-  if (auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
+  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");
+
+    // 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());
       // Don't diagnose an implicit 'operator=='; we will have diagnosed the
       // corresponding defaulted 'operator<=>' already.
       if (!MD->isImplicit()) {
         Diag(MD->getLocation(), diag::err_defaulted_comparison_non_const)
             << (int)DCK << FixItHint::CreateInsertion(InsertLoc, " const");
       }
 
       // Add the 'const' to the type to recover.
       const auto *FPT = MD->getType()->castAs<FunctionProtoType>();
       FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
       EPI.TypeQuals.addConst();
       MD->setType(Context.getFunctionType(FPT->getReturnType(),
                                           FPT->getParamTypes(), EPI));
     }
+  }
+
+  if (FD->getNumParams() != (IsMethod ? 1 : 2)) {
+    // Let's not worry about using a variadic template pack here -- who would do
+    // such a thing?
+    Diag(FD->getLocation(), diag::err_defaulted_comparison_num_args)
+        << int(IsMethod) << int(DCK);
+    return true;
+  }
+
+  const ParmVarDecl *KnownParm = nullptr;
+  for (const ParmVarDecl *Param : FD->parameters()) {
+    QualType ParmTy = Param->getType();
+    if (ParmTy->isDependentType())
+      continue;
+    if (!KnownParm) {
+      auto CTy = ParmTy;
+      // Is it `T const &`?
+      bool Ok = !IsMethod;
+      QualType ExpectedTy{};
+      if (RD)
+        ExpectedTy = Context.getRecordType(RD);
+      if (auto *Ref = CTy->getAs<ReferenceType>()) {
+        CTy = Ref->getPointeeType();
+        if (RD)
+          ExpectedTy.addConst();
+        Ok = true;
+      }
+
+      // Is T a class?
+      if (!Ok) {
+      } else if (RD) {
+        if (!RD->isDependentType() && !Context.hasSameType(CTy, ExpectedTy))
+          Ok = false;
+      } else if (auto *CRD = CTy->getAsRecordDecl())
+        RD = cast<CXXRecordDecl>(CRD);
+      else
+        Ok = false;
+
+      if (Ok)
+        KnownParm = Param;
+      else {
+        // Don't diagnose an implicit 'operator=='; we will have diagnosed the
+        // corresponding defaulted 'operator<=>' already.
+        if (!FD->isImplicit()) {
+          if (RD) {
+            QualType PlainTy = Context.getRecordType(RD);
+            QualType RefTy =
+                Context.getLValueReferenceType(PlainTy.withConst());
+            if (IsMethod)
+              PlainTy = QualType{};
+            Diag(FD->getLocation(), diag::err_defaulted_comparison_param)
+                << int(DCK) << ParmTy << RefTy << int(!IsMethod) << PlainTy
+                << Param->getSourceRange();
           } else {
-    // A non-member function declared in a class must be a friend.
+            assert(!IsMethod && "should know expected type for method");
+            Diag(FD->getLocation(),
+                 diag::err_defaulted_comparison_param_unknown)
+                << int(DCK) << ParmTy << Param->getSourceRange();
+          }
+        }
+        return true;
+      }
+    } else if (!Context.hasSameType(KnownParm->getType(), ParmTy)) {
+      Diag(FD->getLocation(), diag::err_defaulted_comparison_param_mismatch)
+          << int(DCK) << KnownParm->getType() << KnownParm->getSourceRange()
+          << ParmTy << Param->getSourceRange();
+      return true;
+    }
+  }
+
+  assert(RD && "must have determined class");
+  if (IsMethod)
+    ;
+  else if (dyn_cast<CXXRecordDecl>(FD->getLexicalDeclContext()))
+    // In-class, must be a friend decl.
     assert(FD->getFriendObjectKind() && "expected a friend declaration");
+  else {
+    // Out of class, check that FD is a friend.
+    bool Found = false;
+    // Complete RD or error.
+    if (RequireCompleteType(FD->getLocation(), Context.getRecordType(RD),
+                            diag::err_defaulted_comparison_not_friend, int(DCK),
+                            int(1)))
+      return true;
+
+    for (FriendDecl *Friend : RD->friends())
+      if (FD->getCanonicalDecl() ==
+          Friend->getFriendDecl()->getCanonicalDecl()) {
+        Found = true;
+        break;
+      }
+    if (!Found) {
+      Diag(FD->getLocation(), diag::err_defaulted_comparison_not_friend)
+          << int(DCK) << int(0) << RD;
+      Diag(RD->getCanonicalDecl()->getLocation(), diag::note_declared_at);
+      return true;
+    }
   }
 
   // C++2a [class.eq]p1, [class.rel]p1:
   //   A [defaulted comparison other than <=>] shall have a declared return
   //   type bool.
   if (DCK != DefaultedComparisonKind::ThreeWay &&
       !FD->getDeclaredReturnType()->isDependentType() &&
       !Context.hasSameType(FD->getDeclaredReturnType(), Context.BoolTy)) {
void Sema::DefineDefaultedComparison(SourceLocation UseLoc, FunctionDecl *FD,
 
   SynthesizedFunctionScope Scope(*this, FD);
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(UseLoc);
 
   {
     // Build and set up the function body.
-    CXXRecordDecl *RD = cast<CXXRecordDecl>(FD->getLexicalParent());
+    // The first parameter has type maybe-ref-to maybe-const T, use that to get
+    // the type of the class being compared.
+    auto PT = FD->getParamDecl(0)->getType();
+    CXXRecordDecl *RD = PT.getNonReferenceType()->getAsCXXRecordDecl();
     SourceLocation BodyLoc =
         FD->getEndLoc().isValid() ? FD->getEndLoc() : FD->getLocation();
     StmtResult Body =
         DefaultedComparisonSynthesizer(*this, RD, FD, DCK, BodyLoc).build();
     if (Body.isInvalid()) {
       FD->setInvalidDecl();
       return;
     }
if (!DefKind &&
       (!FD->isDependentContext() ||
        (!isa<CXXConstructorDecl>(FD) &&
         FD->getDeclName().getCXXOverloadedOperator() != OO_Equal))) {
     Diag(DefaultLoc, diag::err_default_special_members)
         << getLangOpts().CPlusPlus20;
     return;
   }
 
-  if (DefKind.isComparison() &&
-      !isa<CXXRecordDecl>(FD->getLexicalDeclContext())) {
-    Diag(FD->getLocation(), diag::err_defaulted_comparison_out_of_class)
-        << (int)DefKind.asComparison();
-    return;
-  }
-
   // Issue compatibility warning. We already warned if the operator is
   // 'operator<=>' when parsing the '<=>' token.
   if (DefKind.isComparison() &&
       DefKind.asComparison() != DefaultedComparisonKind::ThreeWay) {
     Diag(DefaultLoc, getLangOpts().CPlusPlus20
                          ? diag::warn_cxx17_compat_defaulted_comparison
                          : diag::ext_defaulted_comparison);
   }
 
   FD->setDefaulted();
   FD->setExplicitlyDefaulted();
 
   // Defer checking functions that are defaulted in a dependent context.
   if (FD->isDependentContext())
     return;
 
   // Unset that we will have a body for this function. We might not,
   // if it turns out to be trivial, and we don't need this marking now
   // that we've marked it as defaulted.
   FD->setWillHaveBody(false);
 
-  // If this definition appears within the record, do the checking when
-  // the record is complete. This is always the case for a defaulted
-  // comparison.
-  if (DefKind.isComparison())
+  // If this is a comparison's defaulted definition within the record, do
+  // the checking when the record is complete.
+  if (DefKind.isComparison() && isa<CXXRecordDecl>(FD->getLexicalDeclContext()))
     return;
-  auto *MD = cast<CXXMethodDecl>(FD);
 
+  // If this member fn was defaulted on its first declaration, we will have
+  // already performed the checking in CheckCompletedCXXClass. Such a
+  // declaration doesn't trigger an implicit definition.
+  if (isa<CXXMethodDecl>(FD)) {
     const FunctionDecl *Primary = FD;
     if (const FunctionDecl *Pattern = FD->getTemplateInstantiationPattern())
       // Ask the template instantiation pattern that actually had the
       // '= default' on it.
       Primary = Pattern;
-
-  // If the method was defaulted on its first declaration, we will have
-  // already performed the checking in CheckCompletedCXXClass. Such a
-  // declaration doesn't trigger an implicit definition.
     if (Primary->getCanonicalDecl()->isDefaulted())
       return;
+  }
+
+  if (DefKind.isComparison()) {
+    if (CheckExplicitlyDefaultedComparison(nullptr, FD, DefKind.asComparison()))
+      FD->setInvalidDecl();
+    else
+      DefineDefaultedComparison(DefaultLoc, FD, DefKind.asComparison());
+  } else {
+    auto *MD = cast<CXXMethodDecl>(FD);
 
-  // FIXME: Once we support defining comparisons out of class, check for a
-  // defaulted comparison here.
     if (CheckExplicitlyDefaultedSpecialMember(MD, DefKind.asSpecialMember()))
       MD->setInvalidDecl();
     else
       DefineDefaultedFunction(*this, MD, DefaultLoc);
   }
+}
 
 static void SearchForReturnInStmt(Sema &Self, Stmt *S) {
   for (Stmt *SubStmt : S->children()) {
     if (!SubStmt)
       continue;
     if (isa<ReturnStmt>(SubStmt))
       Self.Diag(SubStmt->getBeginLoc(),
                 diag::err_return_in_constructor_handler);

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

 // RUN: %clang_cc1 -std=c++2a -verify %s
 
 struct B {};
-bool operator==(const B&, const B&) = default; // expected-error {{equality comparison operator can only be defaulted in a class definition}} expected-note {{candidate}}
-bool operator<=>(const B&, const B&) = default; // expected-error {{three-way comparison operator can only be defaulted in a class definition}}
 
 template<typename T = void>
   bool operator<(const B&, const B&) = default; // expected-error {{comparison operator template cannot be defaulted}}
 
 struct A {
   friend bool operator==(const A&, const A&) = default;
-  friend bool operator!=(const A&, const B&) = default; // expected-error {{invalid parameter type for defaulted equality comparison operator; found 'const B &', expected 'A' or 'const 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}}
 
   bool operator<(const A&) const;
   bool operator<=(const A&) const = default;
   bool operator==(const A&) const volatile && = default; // surprisingly, OK
   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>
     bool operator==(const A&) const = default; // expected-error {{comparison operator template cannot be defaulted}}
 };
 
-// FIXME: The wording is not clear as to whether these are valid, but the
-// intention is that they are not.
-bool operator<(const A&, const A&) = default; // expected-error {{relational comparison operator can only be defaulted in a class definition}}
-bool A::operator<(const A&) const = default; // expected-error {{can only be defaulted in a class definition}}
-
 template<typename T> struct Dependent {
   using U = typename T::type;
   bool operator==(U) const = default; // expected-error {{found 'Dependent<Bad>::U'}}
   friend bool operator==(U, U) = default; // expected-error {{found 'Dependent<Bad>::U'}}
 };
 
 struct Good { using type = const Dependent<Good>&; };
 template struct Dependent<Good>;
struct A {
     friend bool operator==(A &, A &); // expected-note {{would lose const qualifier}}
   };
   struct B {
     A a; // expected-note {{no viable three-way comparison}}
     friend bool operator==(B, B) = default; // ok
     friend bool operator==(const B&, const B&) = default; // expected-warning {{deleted}}
   };
 }
+
+namespace p2085 {
+// out-of-class defaulting
+
+struct S1 {
+  bool operator==(S1 const &) const;
+};
+
+bool S1::operator==(S1 const &) const = default;
+
+bool F1(S1 &s) {
+  return s != s;
+}
+
+struct S2 {
+  friend bool operator==(S2 const &, S2 const &);
+};
+
+bool operator==(S2 const &, S2 const &) = default;
+bool F2(S2 &s) {
+  return s != s;
+}
+
+struct S3 {};                                      // expected-note{{here}}
+bool operator==(S3 const &, S3 const &) = default; // expected-error{{not a friend}}
+
+struct S4;                                         // expected-note{{forward declaration}}
+bool operator==(S4 const &, S4 const &) = default; // expected-error{{not a friend}}
+
+struct S5;                         // expected-note 3{{forward declaration}}
+bool operator==(S5, S5) = default; // expected-error{{not a friend}} expected-error 2{{has incomplete type}}
+
+enum e {};
+bool operator==(e, int) = default; // expected-error{{expected class or reference to a constant class}}
+
+bool operator==(e *, int *) = default; // expected-error{{must have at least one}}
+
+} // namespace p2085

clang/test/CodeGenCXX/p2085.cpp

+// RUN: %clang_cc1 --std=c++20 %s -emit-llvm -o - -triple x86_64-linux | FileCheck %s
+
+namespace std {
+struct strong_ordering {
+  int n;
+  constexpr operator int() const { return n; }
+  static const strong_ordering equal, greater, less;
+};
+constexpr inline strong_ordering strong_ordering::equal = {0};
+constexpr inline strong_ordering strong_ordering::greater = {1};
+constexpr inline strong_ordering strong_ordering::less = {-1};
+} // namespace std
+
+struct Space {
+  int i, j;
+
+  std::strong_ordering operator<=>(Space const &other) const;
+  bool operator==(Space const &other) const;
+};
+
+// Make sure these cause emission
+std::strong_ordering Space::operator<=>(Space const &other) const = default;
+// CHECK-LABEL: define{{.*}} @_ZNK5SpacessERKS_
+bool Space::operator==(Space const &) const = default;
+// CHECK-LABEL: define{{.*}} @_ZNK5SpaceeqERKS_
+
+struct Water {
+  int i, j;
+
+  std::strong_ordering operator<=>(Water const &other) const;
+  bool operator==(Water const &other) const;
+};
+
+// Make sure these do not cause emission
+inline std::strong_ordering Water::operator<=>(Water const &other) const = default;
+// CHECK-NOT: define{{.*}} @_ZNK5WaterssERKS_
+inline bool Water::operator==(Water const &) const = default;
+// CHECK-NOT: define{{.*}} @_ZNK5WatereqERKS_

clang/www/cxx_status.html

</tr>
         <td><a href="https://wg21.link/p1959r0">P1959R0</a></td>
       </tr>
       <tr> <!-- from Prague -->
         <td><a href="https://wg21.link/p2002r1">P2002R1</a></td>
         <td class="partial" align="center">Partial</td>
       </tr>
       <tr>
         <td><a href="https://wg21.link/p2085r0">P2085R0</a></td>
-        <td class="none" align="center">No</td>
+        <td class="unreleased" align="center">Clang 13</td>
       </tr>
     <tr>
       <td>Access checking on specializations</td>
       <td><a href="https://wg21.link/p0692r1">P0692R1</a></td>
       <td class="partial" align="center">Partial</td>
     </tr>
     <tr>
       <td>Default constructible and assignable stateless lambdas</td>