Page MenuHomePhabricator

[ASTImporter] Improved import of TypeSourceInfo (TypeLoc)
AcceptedPublic

Authored by martong on Dec 4 2019, 7:53 AM.

Details

Summary

A new TypeLocImporter is created to import the TypeLoc values from
TypeSourceInfo. The previous "trivial TypeSourceInfo" is replaced.

Event Timeline

martong created this revision.Dec 4 2019, 7:53 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
balazske added inline comments.Dec 4 2019, 8:25 AM
clang/lib/AST/ASTImporter.cpp
8109

I think this should have a less-generic name for example TypeLocImporter_IMPORT.

martong updated this revision to Diff 232336.Dec 5 2019, 6:25 AM

IMPORT -> IMPORT_TYPE_LOC

martong marked an inline comment as done.Dec 5 2019, 6:27 AM

Finally, this FIXME is addressed. Thanks! Are you going to add any tests in the following patches?
It looks almost good to me, but I have a question inline.

clang/lib/AST/ASTImporter.cpp
8109

IMPORT_TYPE_LOC_ELEMENT_OR_RETURN_ERROR.

8238

Does this import interacts well with type loc import partially done at L3258 (VisitFunctionDecl)? Should we merge them?

martong marked an inline comment as done.Dec 12 2019, 1:06 PM

Are you going to add any tests in the following patches?

I don't have any tests for now, but in a later patch we can create them.

clang/lib/AST/ASTImporter.cpp
8238

I think we should check here whether the given ParmVarDecl had been imported previously and if yes then set that with setParam, otherwise it could be set to a nullptr. @balazske what do you think?

balazske added inline comments.Jan 6 2020, 3:05 AM
clang/lib/AST/ASTImporter.cpp
8238

Probably we could get the FunctionTypeLoc from TInfo (this is the first TypeLoc there?) and set the parameters (that are set to nullptr at line 8055), at this loop in VisitFunctionDecl:

// Set the parameters.
for (auto *Param : Parameters) {
  Param->setOwningFunction(ToFunction);
  ToFunction->addDeclInternal(Param);
}
ToFunction->setParams(Parameters);
martong marked 3 inline comments as done.Jan 15 2020, 8:15 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
8238

Guys, I moved the TypeLoc import part of VisitFunctionDecl to here. But, this is possible only if first we import the parameters and only then we import the TypeLoc (TypeSourceInfo) in VisitFunctionDecl, so I made that reorganization too.

martong updated this revision to Diff 238267.Jan 15 2020, 8:15 AM
martong marked an inline comment as done.
  • Move the setting of parameters of FunctionProtoTypeLoc to TypeLocImporter
  • Add tests for TypeLoc import in case of functions
a_sidorin accepted this revision.Jan 19 2020, 9:53 AM

The patch looks good. Thanks! I would prefer to see more tests for other TypeLoc cases, but I think they could be added in the following patches.
@shafik Shafik, do you have any suggestions for this patch?

This revision is now accepted and ready to land.Jan 19 2020, 9:53 AM
martong edited reviewers, added: teemperor; removed: a.sidorin.Jan 27 2020, 8:30 AM

@teemperor , @shafik
Guys, could you please take a look on this patch? I don't think that type locations could have any effects on LLDB, but one may never know, so, it is better to have your review too.
Thanks!

A few comments and I would like @teemperor to give this a look as well but it looks good to me.

clang/lib/AST/ASTImporter.cpp
3281

I am curious, why move this chunk of code up?

clang/unittests/AST/ASTImporterTest.cpp
5850

Maybe I am missing it but these tests don't seem like they cover all the visit methods of TypeLocImporter.

My only problem is the reimplementation of the default visit methods with (from what I can tell) the same behavior as the default. If you delete all of these then that would also fix Shafik's point that all the custom dispatch code is technically untested in the unit test (I think not testing the dispatch code from the TypeLocVisitor is fine).

clang/lib/AST/ASTImporter.cpp
8133

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.

balazske added inline comments.Tue, Feb 25, 2:18 AM
clang/lib/AST/ASTImporter.cpp
3281

See comments at line 8238?

clang/unittests/AST/ASTImporterTest.cpp
5850

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.

balazske added inline comments.Tue, Feb 25, 7:32 AM
clang/lib/AST/ASTImporter.cpp
8133

It is really better to remove these redundant visit branches.

Thanks guys for the review and the comments! :) I am going to address all of them, but only during next week, I am on vacation for the rest of this week.