[PATCH] D38843: [ASTImporter] Support importing CXXPseudoDestructorExpr
Aleksei Sidorin via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 13 09:20:33 PST 2017
a.sidorin added a comment.
Hello Peter,
Patch is almost OK but there are some minor issues.
================
Comment at: lib/AST/ASTImporter.cpp:5549
+ Expr *BaseE = Importer.Import(E->getBase());
+ if (!BaseE)
+ return nullptr;
----------------
szepet wrote:
> xazax.hun wrote:
> > 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?
> 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.
Expr *getBase() const { return cast<Expr>(Base); }
This means it is always non-null, I guess.
================
Comment at: lib/AST/ASTImporter.cpp:5552
+
+ TypeSourceInfo *ScopeInfo = Importer.Import(E->getScopeTypeInfo());
+
----------------
Source ScopeInfo can be null but we still need to check the result.
================
Comment at: unittests/AST/ASTImporterTest.cpp:546
+ "void declToImport(int *p) {"
+ "p->T::~T();"
+ "}",
----------------
# Needs to be aligned.
# 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)?
https://reviews.llvm.org/D38843
More information about the cfe-commits
mailing list