[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:79
+    for (auto R : D->getFirstDecl()->redecls()) {
+      if (R != D->getFirstDecl())
+        Redecls.push_back(R);
----------------
a.sidorin wrote:
> Does this code mean that we can get R == getFirstDecl() in the middle of the import chain?
No.
`D->redecls()` gives a list of decls which always starts with D  and the next item is actually the previous item in the order of source locations. But this is a circular list, so once we reached the first item then the next item will be the last in the order of the source locations.
So, `D->getFirstDecl()->redecls()` gives a reversed list which has D as the first element. We must make D as the last, so we can easily reverse.



================
Comment at: lib/AST/ASTImporter.cpp:2340
+         FunctionDecl::TK_FunctionTemplateSpecialization);
+  auto *FTSInfo = FromFD->getTemplateSpecializationInfo();
+  auto *Template = cast_or_null<FunctionTemplateDecl>(
----------------
a.sidorin wrote:
> This code should be unified with `ImportTemplateInformation()` to avoid duplication.
OK, I've refactored the common code into a new import function.


================
Comment at: lib/AST/ASTImporter.cpp:2363
+  // declarations starting from the canonical decl.
+  for (;RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+    Importer.Import(*RedeclIt);
----------------
a.sidorin wrote:
> Can we just import `getPreviousDecl()`?
In short, we can't. 

The `Redeclarable` class provides the `getPreviousDecl()` function and the `redecls()`range.
The list of the declarations is singly linked circular list and the directions goes backwards.
E.g.:
```
  ///  #1 int f(int x, int y = 1); // previousDecl: <pointer to #3>
  ///  #2 int f(int x = 0, int y); // previousDecl: <pointer to #1>
  ///  #3 int f(int x, int y) { return x + y; } // previousDecl: <pointer to #2>
```
I think, it is very important to preserve the order of the Decls as it was in the from context. Keeping the order makes the imported AST to be structurally similar to the original AST. Also, the canonical Decl in the "From" context will be the canonical in the "To" context too (when we import to an empty context).

During the import of #2 if we used `getPreviousDecl` then we would import first #1 and then #2. The order would be fine. But, during the import of #2 there is no way to import #3, because we can't go forward in the linked list. We could however import #3 during the import of #1, but that would result a #3, #1, #2 order (i.e. a redecl chain where #3 is the canonical decl).

Alternatively we could start the import from the most recent decl (i.e the last) and then recursively import the previous decl up to the first decl, but then we should pass an extra parameter through the function calls of ASTImporter to avoid jumping back to the last decl if we are in the middle of a chain. We could perhaps follow this state in a member variable too, but I consider this way too complex and error prone.

This is the exact reason why we need the `getCanonicalForwardRedeclChain()` function which provides a list of declarations starting from the first (canonical) decl and ends with the latest redecl, this provides the same list for each element in the redecl chain.


================
Comment at: lib/AST/ASTImporter.cpp:2364
+  for (;RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+    Importer.Import(*RedeclIt);
+  assert(*RedeclIt == D);
----------------
a.sidorin wrote:
> Unchecked import result, is it intentional?
Good catch, thanks! Changed it to check the result.


================
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())
----------------
a.sidorin wrote:
> Has removed comment become irrelevant? (Same below)
`// FIXME: Actually try to merge the body and other attributes.`
This FIXME comment (and some other comments) is in strong contrast to all the ODR diagnostics implemented in ASTImporter. Actually, (generally in the ASTImporter) rather than merging two definitions we diagnose an ODR violation.
Perhaps the ASTImporter is used or had been used previously to merge two compatible function or class definitions. But having two structurally equivalent definitions is against the ODR. 
I think we should follow the ODR violation approach here and we should not merge two function body ever.


================
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)
----------------
a.sidorin wrote:
> 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?
When we start with a middle element, then once we reach the first redecl then this loop will import the rest of the chain. So, this means when we are going back in the call stack then some Import calls will just return with the already imported decls. This way, we handle with the same code the import of any element in the chain.
This may seem like a recursive madness, but I hope it makes sense.

You may wonder if this will be quadratic in the length of the declaration chain? Will we import each member of the redecl chain and for each member of the chain will we enumerate all the decls in the declaration chain?
The answer is No, because once a member is imported then all other import calls return early with the already imported decl. Consider the import of `C` from this chain `A<-B<-C`.
```
VisitFunctionDecl(C)
  VisitFunctionDecl(A) // A is imported before any other recursive import to the chain 
    VisitFunctionDecl(B)
       Import(A) // A is already imported, early return
       VisitFunctionDecl(C) 
          Import(A) // A is already imported, early return
          Import(B) // B is already imported, early return
          // C is imported
```
Each VisitFunctionDecl calls `getCanonicalForwardRedeclChain` which is O(`n`). And we go once backward the chain and from the first decl we go forward, so we call `getCanonicalForwardRedeclChain`at maximum 2`n` times . But 99% of the time `n` <= 2. `n` is the number of decls in the redecl chain in the actual "From" context.
Also, we plan to implement the squashing of equivalent prototypes in the future, which will limit  the length of the chains.


================
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
----------------
a.sidorin wrote:
> Why did the order change?
We provide diagnostics during the call of `isStructuralMatch` check. Now the order of the import changed and `isStructuralMatch` is called during the import, thus the diagnostic is emitted later.


================
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);
----------------
a.sidorin wrote:
> We can replace EXPECT_TRUE with negations with EXPECT_FALSE.
Good point, thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D47532