rsmith added a comment.
Do we really need two different notions of "definition" and "odr definition"
here? What goes wrong if we always treat the "instantiation of a friend
function definition" case as being a definition?
================
Comment at: include/clang/AST/Decl.h:1789
+ // function is not used yet, but the declaration is considered a
+ // definition, it does not allow other definition of this function.
+ // - it does not have a user specified body, but it does not allow
----------------
it -> and
================
Comment at: include/clang/AST/Decl.h:1829-1830
+ /// Returns true if the function is defined and other definitions are not
+ /// allowed in the redeclaration chain.
+ ///
----------------
This will not be true in the presence of modules: we will ensure that at most
one declaration has a body, but multiple declarations can be instantiated from
a defined member function of a class template, for instance. So I think the
constraint is that you're not allowed to add *more* such definitions.
================
Comment at: include/clang/AST/Decl.h:1842
+
+ /// Returns true if this declaration is a definition is the sense of ODR
+ /// checks.
----------------
is the sense -> in the sense
================
Comment at: include/clang/AST/Decl.h:1848
+ return true;
+ if (FunctionDecl *Original = getInstantiatedFromMemberFunction())
+ return Original->isOdrDefined();
----------------
I think this will do the wrong thing for an explicit specialization of a member
of a class template:
```
template<typename T> struct A { void f() {}; };
template<> void A<int>::f();
```
Here, `A<int>::f()` is "instantiated from a member function" but it's not a
definition, not even in the ODR sense. I think it would suffice to also check
whether `Original` is a friend declaration here.
================
Comment at: lib/AST/Decl.cpp:2538
+ if (I->isThisDeclarationADefinition()) {
+ Definition = I->IsDeleted ? I->getCanonicalDecl() : I;
+ return true;
----------------
It seems incorrect to me for `isThisDeclarationADefinition()` to return `true`
on a redeclaration of a deleted function: only the first declaration should be
considered to be a definition in that case. (This special case should be in
`isThisDeclarationADefinition()`, not in `isDefined()`.)
================
Comment at: lib/Sema/SemaDecl.cpp:11882
+
+ if (FD == Definition)
+ return;
----------------
This looks wrong to me: `CheckForFunctionRedefinition` is intended to be called
*before* marking the function in question as a definition. If you call it
afterwards, then the `isOdrDefined` call won't work properly because it will
always check to see whether `FD` is a definition before checking any other
declaration. So any case that reaches here with `FD` being a definition seems
like a bug. I expect we're only getting here in that case due to the missing
check for friend in `isThisDeclarationAnOdrDefinition`.
https://reviews.llvm.org/D30170
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
