[PATCH] D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc)

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:3281
 
+  // Import the function parameters.
+  SmallVector<ParmVarDecl *, 8> Parameters;
----------------
balazske wrote:
> shafik wrote:
> > I am curious, why move this chunk of code up?
> See comments at line 8238?
I moved the TypeLoc import part of VisitFunctionDecl to VisitFunctionTypeLoc. But, that was possible only if first we import the parameters and only then we import the TypeLoc (TypeSourceInfo) in VisitFunctionDecl, so I made that reorganization here.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8133
+  Error VisitTypedefTypeLoc(TypedefTypeLoc From) {
+    return VisitTypeSpecTypeLoc(From);
+  }
----------------
balazske wrote:
> teemperor wrote:
> > I know the ASTImporter is doing these reimplementation of the default visitor behavior quite often, but I feel in this case it would be much more readable to omit all these default implementations and just let the TypeLocVisitor do the dispatch to parent class logic. Especially since from what I understand it's unlikely that we need custom logic for all these TypeLoc subclasses in the future? It would also make this code less fragile in case the inheritance hierarchy every gets another intermediate class that might require special handling.
> It is really better to remove these redundant visit branches.
OK, indeed we don't need them. So, I removed these default implementations.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5850
+
+TEST_P(ImportTypeLoc, Function) {
+  Decl *FromTU = getTuDecl(
----------------
balazske wrote:
> shafik wrote:
> > Maybe I am missing it but these tests don't seem like they cover all the visit methods of `TypeLocImporter`.
> It could be useful to add some kind of generic imported AST similarity tests. These can test for things that should match in the imported and original AST, for example source locations.
> (The AST does not always match after import but often.) Such a test can catch many import errors with objects that have no specific test.
> The test in https://reviews.llvm.org/D60463 is somewhat similar but it uses text dump of the AST, this can be improved. Actually that test was used to discover the import problems of `SourceLocation` and `TypeSourceInfo`.
I added tests only for the non-trivial cases like FunctionTypeLoc, in that case we moved some code out from `VisitFunctionDecl` to the typeloc importer. 

All the other branches are very trivial and type loc import happens only in the type loc importer. So, I think it would be the best if we could test them in a generic way as @balazske suggests: we could extend the existing unittests to check for typelocations too. But that is not that trivial to do (may require another typeloc visitation in the test, I don't know yet). I'd rather do that in a separate patch, if that's okay.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71018/new/

https://reviews.llvm.org/D71018