[PATCH] D21508: Make friend function template definition available if class is instantiated.

[Richard Smith via cfe-commits]

rsmith added a comment.

The model that the C++ standard seems to have settled on is that a friend function declaration that's instantiated from a definition is considered to be a definition, even if we've not instantiated its body yet. I think we should try to directly implement that rule.



================
Comment at: lib/AST/DeclTemplate.cpp:292-311
+FunctionTemplateDecl *FunctionTemplateDecl::getDefinition() const {
+  for (auto *R : redecls()) {
+    FunctionTemplateDecl *F = cast<FunctionTemplateDecl>(R);
+    if (F->isThisDeclarationADefinition())
+      return F;
+
+    // If template does not have a body, probably it is instantiated from
----------------
It seems to me that we're getting something fundamentally wrong in the way we model friend function definitions where we've instantiated the declaration but not the definition. Here's a case we get wrong on the non-function-template side:

  template<typename T> struct X { friend void f() {} };
  X<int> xi;
  void f() {}

Line 3 should be ill-formed here due to redefinition, but we don't detect that because we don't believe that the declaration we instantiated on line 2 is a definition. That seems to be the heart of the problem.

Instead of adding this function, can you try changing `FunctionDecl::isThisDeclarationADefinition` to contain the above logic (that is: if this function is a friend that was instantiated from a definition, then it's a definition even if we don't have a body yet)?


================
Comment at: lib/Sema/SemaDecl.cpp:8856
+          (NewTemplateDecl->getFriendObjectKind() != Decl::FOK_None ||
+           OldTemplateDecl->getFriendObjectKind() != Decl::FOK_None))
+        if (FunctionTemplateDecl *NewDef = NewTemplateDecl->getDefinition())
----------------
This doesn't look right; the `OldTemplateDecl` might be `FOK_None` but could still have a definition as a `friend`.

I'm not convinced this is the right place for this check. There are two different cases here:

1) The redefinition error is introduced by parsing a definition in the source code. That should already be handled by `ActOnStartOfFunctionDef`.
2) The redefinition error is introduced by instantiating a friend function template declaration (that has a definition).

I think check (2) belongs in the template instantiation code rather than here.  In fact, at the end of `TemplateDeclInstantiator::VisitFunctionDecl`, there is an attempt to enforce the relevant rule; it just doesn't get all the cases right.


================
Comment at: lib/Sema/SemaDecl.cpp:8857-8863
+        if (FunctionTemplateDecl *NewDef = NewTemplateDecl->getDefinition())
+          if (FunctionTemplateDecl *OldDef = OldTemplateDecl->getDefinition()) {
+            Diag(NewDef->getLocation(), diag::err_redefinition)
+                << NewDef->getDeclName();
+            Diag(OldDef->getLocation(), diag::note_previous_definition);
+            Redeclaration = false;
+          }
----------------
You should use `CheckForFunctionRedefinition` to catch this.


https://reviews.llvm.org/D21508