[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:88
+  llvm::SmallVector<Decl*, 2> getCanonicalForwardRedeclChain(Decl* D) {
+    // Currently only FunctionDecl is supported
+    auto FD = cast<FunctionDecl>(D);
----------------
balazske wrote:
> Assert for FunctionDecl?
`cast` itself will assert if it is not a `FunctionDecl`. (`dyn_cast` is the other formula which can result with a nullptr.)


================
Comment at: unittests/AST/ASTImporterTest.cpp:1745
+TEST_P(ImportFunctions, ImportDefinitions) {
+  auto Pattern = functionDecl(hasName("f"));
+
----------------
balazske wrote:
> This test imports the definition two times, the second should result in error. The import does not check the function body for similarness. Probably a check can be made for the result of the second import.
I don't think this should result an error, because the second definition has the very same type as the first. And also the body is structurally equivalent too, but we do not check that. Perhaps, on a long term we should check the body too (?).

I think the importer should work in this way: if there is already a definition then just simply use that and omit any subsequent definitions if they are the same definitions (or report error if the body is different).
The key principle I followed is this: we can have arbitrary number of prototypes but only one definition.


Repository:
  rC Clang

https://reviews.llvm.org/D47532