Hi Yaron!

This would make sense to me. Let's wait for Richard's opinion. This is a busy week because of the C++ standards committee meeting.

Cheers, Vassil

I believe this fixes two FIXME in SemaTemplate/friend-template.cpp:

// FIXME: the redeclaration note ends up here because redeclaration
// lookup ends up finding the friend target from X3<int>.

// FIXME: As with cases above, the note here is on an unhelpful declaration,
// and should point to the declaration of G within F.

I don't think the proposed change is correct; we should use something like

if (getFriendObjectKind() > OldD->getFriendObjectKind() &&
!isThisDeclarationADefinition())
return false;

The > is needed so that a non-friend can properly replace a friend, and a visible friend can properly replace an invisible friend. The definition check is needed for cases like:

void f();
struct A {
[[noreturn]] void f() { exit(1); }
};
void g() { f(); }

... where we want the 'noreturn' attribute to be visible in g().

Beyond that, I'm concerned that this may lead to us silently losing GNU attributes (or other properties that might be altered by the friend declaration) in cases like:

void f();
struct A {
friend attribute((deprecated)) void f();
};
void g() { f(); }

... because name lookup would only find the non-friend version of 'f'.

That said, perhaps we should consider rejecting GNU attributes on non-defining friend declarations, in the same way that C++11 attributes are ill-formed on such declarations, as a prerequisite for this change. We'd need to do some investigation to see if there is any significant amount of real code relying on our current behaviour.

This makes sense.

isThisDeclarationADefinition is available for VarDecl, FunctionDecl, TagDecl, FunctionTemplateDecl, ClassTemplateDecl, VarTemplateDecl.

For other Decls which has no meaning for definition and no isThisDeclarationADefinition, is this condition always false or decays to

if (getFriendObjectKind() > OldD->getFriendObjectKind())
return false;

or maybe it makes no sense because they won't have friend kind and hence it will be always false?

maybe it makes no sense because they won't have friend kind and hence it
will be always false?

That seems correct.

https://reviews.llvm.org/D31540

reduced case from Richard Smith!
r300443 breaking internal build of libc++ with modules:

While building module 'x':
In file included from :2:
In file included from ./c.h:1:
./a.h:3:32: error: inline declaration of 'f' follows non-inline definition
template inline void f() {}
^
./a.h:3:32: note: previous definition is here
template inline void f() {}

r300443 was reverted in r300497.

the replacement condition may need to consider modules?

with the reproducer, it prevents this replacement:

this=
FunctionTemplateDecl 0x23515b2ab28 parent 0x23515a1d918 prev 0x23515b2a718 <./a.h:2:12, col:45> col:43 hidden f
|-TemplateTypeParmDecl 0x23515b2a9c8 col:21 col:21 hidden typename depth 0 index 0
`-FunctionDecl 0x23515b2aa78 parent 0x23515a1d918 prev 0x23515a1e578 <col:31, col:45> col:43 hidden f 'void (void)'

OldD=
FunctionTemplateDecl 0x23515b2a718 <./a.h:1:1, col:27> col:25 in (local) x.b hidden f
|-TemplateTypeParmDecl 0x23515a1e460 col:10 col:10 hidden typename depth 0 index 0
`-FunctionDecl 0x23515a1e578 <col:20, col:27> col:25 in (local) x.b hidden f 'void (void)'

causing:

|-FunctionTemplateDecl 0x23515b2ae58 prev 0x23515b2ab28 <line:3:1, col:37> col:32 in (local) x.b hidden f
| |-TemplateTypeParmDecl 0x23515b2ad08 col:10 col:10 hidden typename depth 0 index 0
| -FunctionDecl 0x23515b2adb8 prev 0x23515b2aa78 <col:20, col:37> col:32 hidden f 'void (void)' inline | -CompoundStmt 0x23515b2aea8 <col:36, col:37>

but FunctionDecl 0x23515b2adb8 should have been in (local) x.b same as the FunctionTemplateDecl 0x23515b2ae58.

Richard, what do you think?