This is an archive of the discontinued LLVM Phabricator instance.

Change interpretation of function definition in friend declaration of template class.
ClosedPublic

Authored by sepavloff on Feb 8 2016, 8:20 AM.

Details

Summary

Previously if a file-level function was defined inside befriending
template class, it was treated as defined in the code like:

void func(int);
template<typename T> class C1 {
  friend void func(int) {}
};
void m() {
  func(1);
}

The Standard requests ([temp.friend], p5) that `When a function is defined in
a friend function declaration in a class template, the function is instantiated
when the function is odr-used`. This statement however does not specify which
template is used for the instantiation, if the function is defined in several,
and what parameters are used for the instantiation. The latter is important if
the function implementation uses template parameter, such usage is not prohibited
by the Standard.

Other compilers (gcc, icc) follow viewpoint that body of the function defined
in friend declaration becomes available when corresponding class is instantiated.
This patch implements this viewpoint in clang.

Definitions introduced by friend declarations in template classes are added to
the redeclaration chain of corresponding function, but they do not make the
function defined. Only if such definition corresponds to the template
instantiation, the function becomes defined. Presence of function body is not
enough for function definition anymore.

This change fixes PR17923, PR22307 and PR25848.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff updated this revision to Diff 47207.Feb 8 2016, 8:20 AM
sepavloff retitled this revision from to Change interpretation of function definition in friend declaration of template class..
sepavloff updated this object.
sepavloff added a reviewer: rsmith.
sepavloff added a subscriber: cfe-commits.

Topic of friend function instantiation in C++ Standard discussion group

rsmith edited edge metadata.Mar 18 2016, 7:50 AM

Can we instead not add the function to the redeclaration chain until it's instantiated (like we do if it's dependent)?

sepavloff updated this revision to Diff 51817.Mar 28 2016, 11:58 AM
sepavloff edited edge metadata.

Changed implementation

This implementation uses another approach. Previous version of the patch put friend functions
into redeclaration chains but modified search algorithm so that it skipped such functions. In
this version such functions are not included into redeclaration chains at all.

rsmith added inline comments.Apr 25 2016, 11:55 AM
lib/Sema/SemaDecl.cpp
8611–8612 ↗(On Diff #51817)

Please factor this check out into a suitably-named function, shouldLinkDependentDeclWithPrevious or similar, and pass in OldDecl as well. I imagine we'll want to call this from multiple places (for instance, when handling VarDecls), and I can see a few ways of making it return true in more cases, which would allow us to provide more precise diagnostics in a few more cases.

(For instance, if the old and new declarations are in the same lexical context, we can mark them as redeclarations, and more generally I think we can do so if the new declaration has no more template parameters in scope than the old one did and the old declaration is declared within the current instantiation of the new declaration).

8613 ↗(On Diff #51817)

I don't think it makes sense to check whether the template declaration is a definition. It should not be added to the redeclaration chain regardless of whether it's a definition.

10951–10956 ↗(On Diff #51817)

Is this change necessary? If we're not putting dependent templates into redeclaration chains any more, we shouldn't find an existing definition ...

10962 ↗(On Diff #51817)

... down here.

lib/Sema/SemaDeclCXX.cpp
12663–12673 ↗(On Diff #51817)

What do these changes have to do with avoiding putting the declaration into the redeclaration chain? This looks equivalent to what the following if block will do, except that (a) it uses LookupName instead of LookupQualifiedName (which might be a bug fix but doesn't seem related to whether the context was dependent), and (b) it forgets to set DCScope (which looks like a bug).

sepavloff updated this revision to Diff 56015.May 3 2016, 9:28 AM

Updated patch.

Any feedback?

Thanks,
--Serge

Please also add some testcases for the corresponding case for a friend function template:

template<typename T> void f();
template<typename> struct A {
  template<typename T> void f() {}
};
template<typename> struct B {
  template<typename T> void f() {}
};
A<int> a;
B<int> b; // ill-formed
test/SemaCXX/PR25848.cpp
4 ↗(On Diff #56015)

Maybe use int instead of a pointer to member type here? May as well keep the parts that aren't relevant to the test as simple as possible.

test/SemaCXX/friend2.cpp
77 ↗(On Diff #56015)

"must be" -> "can be" (or maybe "should be")

We're not required to diagnose this; it's a QoI issue.

sepavloff updated this revision to Diff 58983.May 30 2016, 12:06 PM

Updated patch

sepavloff marked 2 inline comments as done.May 30 2016, 12:12 PM

Please also add some testcases for the corresponding case for a friend function template:

template<typename T> void f();
template<typename> struct A {
  template<typename T> void f() {}
};
template<typename> struct B {
  template<typename T> void f() {}
};
A<int> a;
B<int> b; // ill-formed

Templates follow very different path. In the code you provided definitions for functions f are not generated, because they are not used, so diagnostics of such cases require special processing. Patch for template friends will be prepared soon.

sepavloff updated this revision to Diff 59532.Jun 3 2016, 4:46 AM

Fixed test for friend functions.

sepavloff updated this revision to Diff 61220.Jun 19 2016, 12:52 PM

Updated patch

Added handling of FunctionTemplateDecl to shouldLinkDependentDeclWithPrevious. It is not
used now but is required for subsequent patch. Also position comment correctly.

sepavloff updated this revision to Diff 65376.Jul 25 2016, 10:40 AM

Added one more case in shouldLinkDependentDeclWithPrevious

sepavloff updated this revision to Diff 68376.Aug 17 2016, 10:08 AM

Rebased the patch

rsmith added inline comments.Aug 23 2016, 12:34 PM
lib/Sema/SemaDecl.cpp
8657–8658 ↗(On Diff #68376)

I don't think this is right. Given:

template<typename T> void f() {
  extern void g();
  extern void g();
}

... we do want to link the two declarations of g together. We even want to link them to a prior declaration of ::g if there is one, since they have a non-dependent type, otherwise we'll reject cases like:

void g();
constexpr void (*p)() = g;
template<bool> struct X { typedef int type; };
template<typename T> void f() {
  extern void g();
  X<&g == p>::type n; // note: no 'typename' required, not value-dependent,
}                     // must evaluate to 'true' while parsing the template

Perhaps we're trying to make this check too general, and we should instead only make this new logic apply to the case of friend declarations. Are there any cases where a friend declaration in a dependent context that names a non-member function needs to be added to the redeclaration chain?

8671–8673 ↗(On Diff #68376)

This is not an example that reaches this case, because the lexical context of the function is not dependent.

sepavloff marked an inline comment as done.Aug 29 2016, 11:33 AM
sepavloff added inline comments.
lib/Sema/SemaDecl.cpp
8657–8658 ↗(On Diff #68376)

It is convenient to have single function for both friend and forward declarations, as it is called from the point where both cases may be observed.

It looks like there is no mandatory requirement to add a friend declaration in a dependent context to the redeclaration chain, because ill-formed program will be detected during instantiation of enclosing template class. But making so allows to detect some problems earlier.

sepavloff updated this revision to Diff 69595.Aug 29 2016, 11:35 AM

Updated patch

Added missing checks for forward declarations in shouldLinkDependentDeclWithPrevious.
Added test case from an ancient bug report.

sepavloff updated this revision to Diff 69597.Aug 29 2016, 11:39 AM

Added missed test case.

sepavloff updated this revision to Diff 70025.Sep 1 2016, 10:20 AM

Fixed DR136 for template classes

Previously clang created declarations of friend functions declared in template classes even
if that class was not instantiated. It allowed to make check for default arguments required
by DR136. This fix enables the check for the new implementation.

Also extended comment about putting friend function declarations into redeclaration chains.

Putting a friend declaration to the appropriate redeclaration chain is not necessary, any
errors in the declaration will be detected at instantiation time. However proper organization
of the declarations enables early detection of potential problems.

If a friend function is defined inline, it is almost certain that it should not be added to
the redeclaration chain. The following code demonstrates the problem:

void func();
template<typename T> class C1 {
    friend void func() {}
};
template<typename T> class C2 {
    friend void func() {}
}

Putting definitions of the friend functions into the same chain would require additional flag
to distinguish between 'actual' and 'potential' definitions. Operation with the chain becomes
more complicated. Putting only 'actual' definitions saves from the need to modify the mechanism
of redeclaration chain.

Putting non-defining declaration into the chain seems unnecessary. If there is corresponding
declaration at namespace level, as in the code:

void func();
template<typename T> class C1 {
    friend bool func() {}    // <- cannot be redeclared with different return type
};

compiler can detect invalid redeclaration because it finds prior declaration in the translation
unit in this example. Declaration of the friend function does not need to be saved for any check
in this case.

If there is no the function declaration at namespace level, putting non-defining declaration into
the chain can cause problems, for instance:

template<typename T> class C1 {
    friend void func() {}
};
template<typename T> class C2 {
    friend bool func() {}    // <- different return type
}

This code does not violates any requirement of the C++ standard. Only if both templates are
instantiated, code becomes ill-formed. But these friend function declarations cannot be put into
the same redeclaration chain.

So, friend function declarations defined in dependent context that names file-level functions
should not be put into redeclaration chain.

rsmith added inline comments.Sep 1 2016, 2:39 PM
lib/Sema/SemaDecl.cpp
8652 ↗(On Diff #70025)

Most of the logic in this function can be replaced by something much simpler, such as

`return !(D->getFriendObjectKind() && D->getLexicalDeclContext()->isDependentContext());`

Can you try to make such a simplification? Can we use the above as the criterion here?

sepavloff marked an inline comment as done.Sep 1 2016, 11:08 PM
sepavloff added inline comments.
lib/Sema/SemaDecl.cpp
8652 ↗(On Diff #70025)

Indeed, in negative form the condition is much simpler.
Thank you!

sepavloff updated this revision to Diff 70134.Sep 1 2016, 11:11 PM
sepavloff marked an inline comment as done.

Updated patch

Rewrite the condition in shouldLinkDependentDeclWithPrevious in more compact form.

rsmith accepted this revision.Oct 3 2016, 2:55 PM
rsmith edited edge metadata.

A couple of comment nits, then this LGTM. Thanks!

lib/Sema/SemaDecl.cpp
8659–8664 ↗(On Diff #70134)

We accept the second example without your change, and never get here (because we never find a PrevDecl). Just remove that one? The first example alone is enough.

lib/Sema/SemaDeclCXX.cpp
13770–13774 ↗(On Diff #70134)

This is a bit of a weird thing to do, so please add a comment here explaining why we're not just looking at getPreviousDecl(). Something like:

"We can't look at FD->getPreviousDecl() because it may not have been set if we're in a dependent context. If we get this far with a non-empty Previous set, we must have a valid previous declaration of this function."

This revision is now accepted and ready to land.Oct 3 2016, 2:55 PM
This revision was automatically updated to reflect the committed changes.