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

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello Gabor,

This patch is really cool. Unfortunately, right now I don't have enough time for reviewing large patches in a single review. Are you OK with incremental review?



================
Comment at: lib/AST/ASTImporter.cpp:75
+  template <class T>
+  llvm::SmallVector<Decl*, 2>
+  getCanonicalForwardRedeclChain(Redeclarable<T>* D) {
----------------
We can drop `llvm::` prefix for SmallVectors.


================
Comment at: lib/AST/ASTImporter.cpp:78
+    llvm::SmallVector<Decl*, 2> Redecls;
+    for (auto R : D->getFirstDecl()->redecls()) {
+      if (R != D->getFirstDecl())
----------------
Please keep `*` where possible.


================
Comment at: lib/AST/ASTImporter.cpp:79
+    for (auto R : D->getFirstDecl()->redecls()) {
+      if (R != D->getFirstDecl())
+        Redecls.push_back(R);
----------------
Does this code mean that we can get R == getFirstDecl() in the middle of the import chain?


================
Comment at: lib/AST/ASTImporter.cpp:2340
+         FunctionDecl::TK_FunctionTemplateSpecialization);
+  auto *FTSInfo = FromFD->getTemplateSpecializationInfo();
+  auto *Template = cast_or_null<FunctionTemplateDecl>(
----------------
This code should be unified with `ImportTemplateInformation()` to avoid duplication.


================
Comment at: lib/AST/ASTImporter.cpp:2363
+  // declarations starting from the canonical decl.
+  for (;RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+    Importer.Import(*RedeclIt);
----------------
Can we just import `getPreviousDecl()`?


================
Comment at: lib/AST/ASTImporter.cpp:2364
+  for (;RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+    Importer.Import(*RedeclIt);
+  assert(*RedeclIt == D);
----------------
Unchecked import result, is it intentional?


================
Comment at: lib/AST/ASTImporter.cpp:2380
+  // If this is a function template specialization, then try to find the same
+  // existing specialization in the "to" context.  The localUncachedLookup
+  // below will not find any specialization, but would find the primary
----------------
Double space.


================
Comment at: lib/AST/ASTImporter.cpp:2409
                                                 FoundFunction->getType())) {
-            // FIXME: Actually try to merge the body and other attributes.
-            const FunctionDecl *FromBodyDecl = nullptr;
-            D->hasBody(FromBodyDecl);
-            if (D == FromBodyDecl && !FoundFunction->hasBody()) {
-              // This function is needed to merge completely.
-              FoundWithoutBody = FoundFunction;
+              if (D->doesThisDeclarationHaveABody() &&
+                  FoundFunction->hasBody())
----------------
Has removed comment become irrelevant? (Same below)


================
Comment at: lib/AST/ASTImporter.cpp:2609
 
+  // Friend declarations lexical context is the befriending class, but the
+  // semantic context is the enclosing scope of the befriending class.
----------------
declaration's


================
Comment at: lib/AST/ASTImporter.cpp:2612
+  // We want the friend functions to be found in the semantic context by lookup.
+  // FIXME should we handle this genrically in VisitFriendDecl?
+  // In Other cases when LexicalDC != DC we don't want it to be added,
----------------
generically?


================
Comment at: lib/AST/ASTImporter.cpp:2619
+
+  // Import the rest of the chain. I.e. import all subsequent declarations.
+  for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt)
----------------
So, we're importing a redeclaration. It imports its own redeclarations during import. This means that after the first redecl is imported, all other redecls should be imported too. So, do we need to have a loop here?


================
Comment at: test/ASTMerge/class/test.cpp:20
 
+// CHECK: class1.cpp:36:8: warning: type 'F2' has incompatible definitions in different translation units
+// CHECK: class1.cpp:39:3: note: friend declared here
----------------
Why did the order change?


================
Comment at: unittests/AST/ASTImporterTest.cpp:1745
+TEST_P(ImportFunctions, ImportDefinitions) {
+  auto Pattern = functionDecl(hasName("f"));
+
----------------
martong wrote:
> 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.
I think this is a right approach because we have ODR. Unless function signatures are different, only one possible definition should be used.
This situation is quite different from ASTImporter approach because ASTImporter checker for declaration compatibility, not definition compatibility. ASTImporter does not check function bodies for similarity, and I believe it shouldn't.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1838
+  ASSERT_EQ(DeclCounter<FunctionDecl>().match(ToTU, Pattern), 3u);
+  FunctionDecl* ProtoD = FirstDeclMatcher<FunctionDecl>().match(ToTU, Pattern);
   EXPECT_TRUE(!ProtoD->doesThisDeclarationHaveABody());
----------------
Misplaced `*`s appeared. Could you clang-format?


================
Comment at: unittests/AST/ASTImporterTest.cpp:1924
+  ASSERT_EQ(DeclCounter<FunctionDecl>().match(ToTU, Pattern), 2u);
+  EXPECT_TRUE(!ImportedD->doesThisDeclarationHaveABody());
+  auto ToFD = LastDeclMatcher<FunctionDecl>().match(ToTU, Pattern);
----------------
We can replace EXPECT_TRUE with negations with EXPECT_FALSE.


Repository:
  rC Clang

https://reviews.llvm.org/D47532