[PATCH] D58292: Add support for importing ChooseExpr AST nodes.
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 20 16:29:16 PST 2019
aaron.ballman added inline comments.
================
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());
----------------
a_sidorin wrote:
> 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`.
Wow. I really dislike that we basically *have* to hide the type information because it's just too ugly to spell. I understand why we're using `auto` based on your explanation, but it basically means I can't understand what this code is doing without a lot more effort.
Let's keep the `auto`, but let's please stop using type compositions that make it considerably harder to review the code. This feel like a misuse of `std::tuple`.
================
Comment at: lib/AST/ASTImporter.cpp:6160
+ // condition-dependent.
+ bool CondIsTrue = false;
+ if (!E->isConditionDependent())
----------------
a_sidorin wrote:
> aaron.ballman wrote:
> > `bool CondIsTrue = E->isConditionDependent() ? false : E->isConditionTrue();`
> `bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
I like that even better than my suggestion. :-)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58292/new/
https://reviews.llvm.org/D58292
More information about the cfe-commits
mailing list