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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked an inline comment as done.
sepavloff added inline comments.


================
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
----------------
rsmith wrote:
> 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)?
The code snippet miscompiles due to another problem, it is related to friend functions, not function templates, and is addressed by D30170.

> Instead of adding this function, can you try changing FunctionDecl::isThisDeclarationADefinition to contain the above logic

`FunctionDecl::isThisDeclarationADefinition` is used in many places, in some it should not consider uninstantiated bodies. Making special function for such checks makes the fix more compact and reduces the risk to break existing code. For function declarations it is `isThisDeclarationAnOdrDefinition` introduced in D30170, for function templates `isDefined` is created in the updated version of this patch.


================
Comment at: lib/Sema/SemaDecl.cpp:8856
+          (NewTemplateDecl->getFriendObjectKind() != Decl::FOK_None ||
+           OldTemplateDecl->getFriendObjectKind() != Decl::FOK_None))
+        if (FunctionTemplateDecl *NewDef = NewTemplateDecl->getDefinition())
----------------
rsmith wrote:
> 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.
Indeed, marking both `FunctionTemplateDecl` and corresponding `FunctionDecl` as friends was enough to make `TemplateDeclInstantiator::VisitFunctionDecl` work properly.

As for the case (1), the check was moved to `CheckForFunctionRedefinition`, which is eventually called from `ActOnStartOfFunctionDef`.


https://reviews.llvm.org/D21508