martong added inline comments.
================
Comment at: clang/lib/AST/ASTImporter.cpp:1737
// If we are in the process of ImportDefinition(...) for a RecordDecl we
// want to make sure that we are also completing each FieldDecl. There
----------------
`ImportDefinition(...)` here refers to `ASTImporter::ImportDefinition(Decl*)`
and not any of the `ASTNodeImporter::ImportDefinition` funcitons. This is a
very important distinction, because the former is part of the public interface,
while the latter are private implementation functions. Could you please fix
this in the comment?
================
Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+ QualType FromTy = ArrayFrom->getElementType();
+ QualType ToTy = ArrayTo->getElementType();
+
+ FromRecordDecl = FromTy->getAsRecordDecl();
+ ToRecordDecl = ToTy->getAsRecordDecl();
----------------
labath wrote:
> What about 2- or n-dimensional arrays?
@labath, this is a very good question! And made me realize that D71378 is
fundamentally flawed (@shafik, please take no offense). Let me explain.
So, with D71378, we added the `if (ImportedOrErr) { ... }` block to import
definitions specifically of fields' Record types. But we forget to handle
arrays. Now we may forget to handle multidimensional arrays ... and we may
forget to handle other language constructs. So, we would finally end up in
re-implementing the logic of `ASTNodeImporter::VisitFieldDecl`.
So all this should have been handled properly by the preceding import call of
the FieldDecl! Here
```
1735: ExpectedDecl ImportedOrErr = import(From);
```
I have a suspicion that real reason why this import call fails in case of the
public ASTImporter::ImportDefinition() is that we fail to drive through the
import kind (`IDK_Everything`) during the import process.
Below we set IDK_Everything and start a complete import process.
```
8784 if (auto *ToRecord = dyn_cast<RecordDecl>(To)) {
8785 if (!ToRecord->getDefinition()) {
8786 return Importer.ImportDefinition( //
ASTNodeImporter::ImportDefinition !
8787 cast<RecordDecl>(FromDC), ToRecord,
8788 ASTNodeImporter::IDK_Everything);
8789 }
8790 }
```
However, there may be many places where we fail to channel through that we want
to do a complete import. E.g here
```
1957 ImportDefinitionIfNeeded(Base1.getType()->getAsCXXRecordDecl()))
```
we do another definition import and IDK_Everything is not set. So we may have a
wrong kind of import since the "minimal" flag is set.
The thing is, it is really confusing and error-prone to have both the
`ASTImporter::Minimal` flag and the `ImportDefinitionKind`. They seem to be in
contradiction to each other some times.
I think we should get rid of the Minimal flag. And Kind should be either a full
completion (IDK_Everythink) or just a minimal (IDK_Basic). So, I'd scrap the
IDK_Default too. Alternatively we could have a Kind member in
AST**//Node//**Importer.
I think we should go into this direction to avoid similar problems during
CodeGen in the future. WDYT?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86660/new/
https://reviews.llvm.org/D86660
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
