[PATCH] D30170: Function definition may have uninstantiated body

[Richard Smith via Phabricator via cfe-commits]

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