Page MenuHomePhabricator

[ASTImporter] Support importing CXXPseudoDestructorExpr
ClosedPublic

Authored by szepet on Oct 12 2017, 5:14 AM.

Diff Detail

Event Timeline

szepet created this revision.Oct 12 2017, 5:14 AM
xazax.hun added inline comments.Oct 19 2017, 2:36 AM
lib/AST/ASTImporter.cpp
5919

Does E->getBase() guaranteed to return non-null? What happens when this node was constructed using EmptyShell? Shouldn't we check for that somehow? When can that happen?

szepet added inline comments.Oct 19 2017, 6:25 AM
lib/AST/ASTImporter.cpp
5919

The import process of ArraySubscriptExpr and UnaryOperator (and probably more other classes) are not prepared for this case as well. Not sure if this can be encountered in a complete AST.
However, I think a lazy evaluated && operator won't hurt the performance and at least we are going to be prepared for this case.

szepet updated this revision to Diff 119579.Oct 19 2017, 6:26 AM

Checking for importing base updated to (!Imported && From) style.

a.sidorin edited edge metadata.Nov 13 2017, 9:20 AM

Hello Peter,

Patch is almost OK but there are some minor issues.

lib/AST/ASTImporter.cpp
5919
Expr *getBase() const { return cast<Expr>(Base); }

This means it is always non-null, I guess.

5922

Source ScopeInfo can be null but we still need to check the result.

unittests/AST/ASTImporterTest.cpp
600
  1. Needs to be aligned.
  2. Unused templates are common source of test failures on Windows (MSVC). Could you add a template instantiation (or check it works on Windows as expected)?
szepet updated this revision to Diff 124173.Nov 24 2017, 5:22 AM
szepet marked 3 inline comments as done.

Thank you for the review!

Updated according to review comments.

a.sidorin accepted this revision.Nov 26 2017, 5:19 AM

Looks good, thank you!

This revision is now accepted and ready to land.Nov 26 2017, 5:19 AM
This revision was automatically updated to reflect the committed changes.