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());
----------------
aaron.ballman wrote:
> 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`.
After staring at this for longer, I no longer think it's a misuse of
`std::tuple`, just... an unfortunate side-effect of the design of
`importSeq()`. I don't have a good idea for how to make this easier on
reviewers and other people who aren't frequently looking at the AST importing
code. :-(
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58292/new/
https://reviews.llvm.org/D58292
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
