This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Supporting CXXOperatorCallExpr, SizeOfPackExpr, DependentTemplateSpecializationType, DependentSizedArray, CXXTypeidExpr importing.
ClosedPublic

Authored by xazax.hun on Jan 20 2018, 9:25 AM.

Diff Detail

Repository
rC Clang

Event Timeline

xazax.hun created this revision.Jan 20 2018, 9:25 AM

Hello Peter,

Thank you for the patch! It is almost LGTM, just a few minor questions inline.
Am I understand correctly that it is partially based on https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp?
Also, FYI: you can use ASTMerge and clang-import-test facilities for testing. ASTMatchers are completely fine, but usage of imported AST samples as Sema lookup source can uncover some subtle issues in the built AST.

lib/AST/ASTImporter.cpp
536

Redundant newline? (Same below).

5964

This can be merged with VisitCallExpr to avoid code duplication.

xazax.hun edited the summary of this revision. (Show Details)Jan 22 2018, 1:45 PM
xazax.hun updated this revision to Diff 130961.Jan 22 2018, 2:25 PM
xazax.hun marked 2 inline comments as done.
  • Address review comments.

Hello Peter,

Thank you for the patch! It is almost LGTM, just a few minor questions inline.
Am I understand correctly that it is partially based on https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp?
Also, FYI: you can use ASTMerge and clang-import-test facilities for testing. ASTMatchers are completely fine, but usage of imported AST samples as Sema lookup source can uncover some subtle issues in the built AST.

Yeah that is right. One reason I prefer matcher based tests because they tend to be more lightweight, but I think you are right, it is not a bad idea to utilize other tools as well.

xazax.hun updated this revision to Diff 131059.Jan 23 2018, 6:48 AM
  • Added import for CXXTypeidExpr. What is the best way to test this? <typeinfo> header is required for using the typeid operator, but relying on the presence of an STL library in tests is usually considered as a bad practice. Should we mock a typeinfo implementation just for this purpose?
xazax.hun updated this revision to Diff 131065.Jan 23 2018, 7:12 AM
xazax.hun retitled this revision from [ASTImporter] Supporting CXXOperatorCallExpr, SizeOfPackExpr, DependentTemplateSpecializationType, DependentSizedArray importing. to [ASTImporter] Supporting CXXOperatorCallExpr, SizeOfPackExpr, DependentTemplateSpecializationType, DependentSizedArray, CXXTypeidExpr importing..
  • Added test for CXXTypeIdExpr import.
a.sidorin added inline comments.Jan 24 2018, 5:42 AM
lib/AST/ASTImporter.cpp
6409

As I see from usage of getTypeOperandSourceInfo(), it cannot return nullptr. getExprOperand() is used unchecked sometimes too, but not everywhere.

unittests/AST/ASTImporterTest.cpp
657

This will find only the first typeid(). How about something like this:

void declToImport() {"
             "  int x;"
             " auto a = typeid(int), b = typeid(x);"
             "}",
             Lang_CXX, "", Lang_CXX, Verifier,
             functionDecl(has(varDecl(hasName("a"), hasInitializer(cxxTypeidExpr())),
                                      has(varDecl(hasName("b"), hasInitializer(cxxTypeidExpr())));

?

xazax.hun updated this revision to Diff 131406.Jan 25 2018, 2:55 AM
xazax.hun marked 2 inline comments as done.
  • Address review comments.
a.sidorin accepted this revision.Jan 25 2018, 9:00 AM

LGTM, thank you!

This revision is now accepted and ready to land.Jan 25 2018, 9:00 AM
This revision was automatically updated to reflect the committed changes.