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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
