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.

Diff Detail

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.Feb 25 2020, 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.Feb 25 2020, 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.

martong updated this revision to Diff 249106.Mar 9 2020, 8:16 AM
martong marked 10 inline comments as done.
  • Remove default visit functions
clang/lib/AST/ASTImporter.cpp
3281

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.

8133

OK, indeed we don't need them. So, I removed these default implementations.

clang/unittests/AST/ASTImporterTest.cpp
5850

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.

shafik accepted this revision.Mar 9 2020, 10:42 AM

LGTM

balazske added inline comments.Mar 10 2020, 2:01 AM
clang/lib/AST/ASTImporter.cpp
8132

This assert is new related to the old code. Are really all TypeLoc's are implemented (probably analyze with CTU a large project)?

martong marked 2 inline comments as done.Mar 10 2020, 3:26 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
8132

Yeah, I am not going to commit this, until our full CI job is green on the change. That job analyzes Tmux, Redis, Curl, Xercex, Bitcoin and Protobuf and considered as a quite good coverage for the Analyzer.

martong updated this revision to Diff 249350.Mar 10 2020, 6:12 AM
martong marked an inline comment as done.
  • Handle AttributedTypeLoc
martong updated this revision to Diff 250568.Mar 16 2020, 8:33 AM
  • Remove redundant VisitQualifiedTypeLoc and VisitAttributedTypeLoc
martong updated this revision to Diff 250570.Mar 16 2020, 8:37 AM
  • Fix the base commit of this patch
  • Remove redundant VisitQualifiedTypeLoc and VisitAttributedTypeLoc

I realized that the mentioned functions are redundant, because we have a while loop which iterates over all related typelocs (getNextTypeLoc).
Also, it is better to remove the llvm_unreachable because there are many TypeLocs that we just don't handle, e.g. TypeOfExprTypeLoc.

Harbormaster completed remote builds in B49316: Diff 250570.