[PATCH] D30170: Function definition may have uninstantiated body

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/Decl.h:1868-1871
   bool isThisDeclarationADefinition() const {
-    return IsDeleted || Body || IsLateTemplateParsed;
+    return IsDeleted || IsDefaulted || Body || IsLateTemplateParsed ||
+      hasDefiningAttr();
   }
----------------
Can you check in this change and the corresponding changes to `test/SemaCXX/cxx0x-cursory-default-delete.cpp` separately?


================
Comment at: lib/AST/Decl.cpp:2557
+      // have a body until it is used, nevertheless it is a definition. The
+      // following code must produce redeclaration error:
+      //
----------------
redeclaration -> redefinition


================
Comment at: lib/Sema/SemaDecl.cpp:11882
+
+  if (FD == Definition)
+    return;
----------------
sepavloff wrote:
> 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.
Ultimately, I'd like to be able to remove the special-case handling for friend function redefinitions from `TemplateDeclInstantiator::VisitFunctionDecl` and simply call `CheckForFunctionRedefinition` from there (we just need to keep the part that checks for a prior declaration marked as 'used' and instantiates the definition if so). For that to work, this function has to be able to cope with the case where `FD` is an instantiated friend and there is another instantiated friend in the redeclaration chain. In that case, `FD->isOdrDefined()` is going to determine that `FD` itself is the definition and not find the other definition (because `D->redecls()` yields `D` first).

I think the best way to make that work would be to skip `FD` when looping over redeclarations and checking for an existing definition. At that point, `isOdrDefined` is really not a general-purpose mechanism for checking for a definition, and should probably simply be inlined into here.



https://reviews.llvm.org/D30170