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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
