[PATCH] D30170: Function definition may have uninstantiated body

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked 2 inline comments as done.
sepavloff added a comment.

In https://reviews.llvm.org/D30170#761342, @rsmith wrote:

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


It requires 3 additional changes besides update of `isDefined`. All of them implement current behavior of the function and look like:

  @@ -3749,7 +3750,8 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
                                            bool Recursive,
                                            bool DefinitionRequired,
                                            bool AtEndOfTU) {
  -  if (Function->isInvalidDecl() || Function->isDefined())
  +  const FunctionDecl *Def;
  +  if (Function->isInvalidDecl() || (Function->isDefined(Def) && Def->isThisDeclarationADefinition()))
       return;
  
     // Never instantiate an explicit specialization except if it is a class scope

It make sense to replace this weird expression `Function->isDefined(Def) && Def->isThisDeclarationADefinition()` with a call to a new function, something like `isActuallyDefined`. So we anyway have to add a new function for checking function definition. In the case of `IsOdrDefinition` the new function is used in one place only, modified version of `isDefined` requires changes in unrelated code places.

Use of a special variant of `isDefined` dedicated to redefinition checks looks less invasive and more robust. If however there are reasons to use modified `isDefined`, it also can be implemented, there is nothing wrong with such solution.



================
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.
+  ///
----------------
rsmith wrote:
> 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.
Fixed also some comments according to this note.


================
Comment at: include/clang/AST/Decl.h:1848
+      return true;
+    if (FunctionDecl *Original = getInstantiatedFromMemberFunction())
+      return Original->isOdrDefined();
----------------
rsmith wrote:
> 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.
Although this function is not called in this case, its name must agree with its functionality, so changed implementation as you recommend.


================
Comment at: lib/AST/Decl.cpp:2538
+    if (I->isThisDeclarationADefinition()) {
+      Definition = I->IsDeleted ? I->getCanonicalDecl() : I;
+      return true;
----------------
rsmith wrote:
> 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()`.)
Indeed, if a declaration was selected to be a definition, it is strange if `isThisDeclarationADefinition` returns false for it.


================
Comment at: lib/Sema/SemaDecl.cpp:11882
+
+  if (FD == Definition)
+    return;
----------------
rsmith wrote:
> 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`.
`CheckForFunctionRedefinition` is called after the declaration is put to redeclaration chain. At this point the declaration does not have a body, so problem may arise only if the provided declaration is an instantiation. It happens when the method is called from `PerformPendingInstantiations` -> `InstantiateFunctionDefinition` -> `ActOnStartOfFunctionDef` -> `CheckForFunctionRedefinition`. In this case function body is generated for declaration that is already in redeclaration chain. If the chain is in valid state, it does not have duplicate definitions and call to `CheckForFunctionRedefinition` is redundant and this check makes shortcut return.


https://reviews.llvm.org/D30170