a_sidorin added a comment.
Hi Tom,
Thank for the fixes, now the patch looks almost fine. I have a small nit
inline, but I think the patch can land after this fix.
================
Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+ auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+ E->getBuiltinLoc(), E->getRParenLoc(), E->getType());
----------------
aaron.ballman wrote:
> Please don't use `auto` here; the type isn't spelled out in the
> initialization.
Hi Aaron,
importSeq() is a helper targeted to simplify a lot of imports done during AST
node import and related error handling. The type of this `importSeq()`
expression is `Expected<std::tuple<Expr *, Expr *, Expr *, SourceLocation,
SourceLocation, QualType>>`, so I think it's better to leave it as `auto`.
================
Comment at: lib/AST/ASTImporter.cpp:6160
+ // condition-dependent.
+ bool CondIsTrue = false;
+ if (!E->isConditionDependent())
----------------
aaron.ballman wrote:
> `bool CondIsTrue = E->isConditionDependent() ? false : E->isConditionTrue();`
`bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58292/new/
https://reviews.llvm.org/D58292
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
