[PATCH] D38843: [ASTImporter] Support importing CXXPseudoDestructorExpr

[Aleksei Sidorin via Phabricator via cfe-commits]

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