Page MenuHomePhabricator

Diagnose friend function template redefinitions
ClosedPublic

Authored by sepavloff on Jun 19 2016, 12:56 PM.

Details

Summary

Friend function template becomes available if enclosing class template is
instantiated. Now the check for redefinition do not handle correctly such
function template and some redefinitions are not diagnosed. The reason is
the friend function templates in the instantiated class do not have
bodies, as they are not used yet and the search for redefinition does not
take into account.

The change modifies redefinition check so that it can find the friend
function template definitions in instantiated classes. The check is based
on the new function, 'FunctionTemplateDecl::isDefined'. In addition to the
checks made by 'isThisDeclarationADefinition' it checks if the given
declaration has uninstantiated body.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff updated this revision to Diff 61222.Jun 19 2016, 12:56 PM
sepavloff retitled this revision from to Make friend function template definition available if class is instantiated..
sepavloff updated this object.
sepavloff added a reviewer: rsmith.
sepavloff added a subscriber: cfe-commits.
sepavloff updated this revision to Diff 61989.Jun 27 2016, 11:42 AM

Make more correct template function definition lookup

Lookup is made recursive to cover more cases.

sepavloff updated this revision to Diff 74218.Oct 11 2016, 3:22 AM

Updated comments, NFC otherwise.

sepavloff updated this object.Oct 11 2016, 3:24 AM
sepavloff updated this object.Oct 12 2016, 9:51 PM
rsmith edited edge metadata.Oct 24 2016, 5:00 PM

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.

lib/AST/DeclTemplate.cpp
292–311 ↗(On Diff #74218)

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)?

lib/Sema/SemaDecl.cpp
8856 ↗(On Diff #74218)

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.

8857–8863 ↗(On Diff #74218)

You should use CheckForFunctionRedefinition to catch this.

sepavloff marked an inline comment as done.May 22 2017, 11:25 AM
sepavloff added inline comments.
lib/AST/DeclTemplate.cpp
292–311 ↗(On Diff #74218)

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.

lib/Sema/SemaDecl.cpp
8856 ↗(On Diff #74218)

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.

sepavloff updated this revision to Diff 99789.May 22 2017, 11:25 AM
sepavloff edited the summary of this revision. (Show Details)

Updated patch

sepavloff retitled this revision from Make friend function template definition available if class is instantiated. to Diagnose friend function template redefinitions.May 22 2017, 11:28 AM
sepavloff edited the summary of this revision. (Show Details)
sepavloff edited the summary of this revision. (Show Details)

Aligned implementation with D30170.

sepavloff updated this revision to Diff 107091.Jul 18 2017, 6:55 AM

Simplified patch

Updated patch

  • Rebased relative to recent ToT
  • Removed the change in FunctionDecl::getTemplateInstantiationPattern(), as it is not necessary for error detection,
  • Added test for use in module.

Other compilers successfully recognize errors (checked using https://godbolt.org/). For instance, the code:

template<typename T> inline void func_35(T *x);
template<typename T1>
struct C35a {
  template<typename T> friend void func_35(T *x) {}
};
template<typename T1>
struct C35b {
  template<typename T> friend void func_35(T *x) {}
};
C35a<int> v35a;
C35b<int> v35b;

is rejected by MSVC 19:

example.cpp
<source>(7): error C2995: 'void func_37(T *)': function template has already been defined
<source>(1): note: see declaration of 'func_37'
Microsoft (R) C/C++ Optimizing Compiler Version 19.10.25017 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.
Compiler returned: 2

by gcc 7.3:

<source>:7:27: error: redefinition of 'template<class T> void func_37(T*)'
 template<typename T> void func_37(T *x) {}
                           ^~~~~~~
<source>:1:27: note: 'template<class T> void func_37(T*)' previously declared here
 template<typename T> void func_37(T *x);
                           ^~~~~~~
Compiler returned: 1

and icc 18:

<source>(7): error: function template "func_37" has already been defined
  template<typename T> void func_37(T *x) {}
                            ^
compilation aborted for <source> (code 2)
Compiler returned: 2
sepavloff updated this revision to Diff 160298.Aug 13 2018, 1:09 AM

Rebased the patch

sepavloff updated this revision to Diff 176763.Dec 4 2018, 10:40 PM

Updated patch

The fix for https://bugs.llvm.org/show_bug.cgi?id=39742 put a test
case, which is not a valid code. The error is detected with this
patch, tests are updated accordingly.

rsmith accepted this revision.Dec 5 2018, 11:28 AM
rsmith marked an inline comment as done.

Thanks (and sorry for dropping the ball on this review).

This revision is now accepted and ready to land.Dec 5 2018, 11:28 AM
This revision was automatically updated to reflect the committed changes.