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:
> > 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. :-(
> I think it is a case where the type is not even important. With C++17, we
> would just write:
> ```auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), ...)
> if (Imp)
> return Imp.takeError();
> auto [ToCond, ToLHS, ToRHS] = *Imp;
> ```
> The exact type is not really needed here. However, if you have any ideas on
> how to improve this and make the type more explicit - they are always welcome.
I actually don't find the structured binding version to be any more readable
than the wrapped-tuple version -- either way, I don't see the types. What makes
this current code somewhat-okay to me is the `std::tie()` below; that at least
lets me figure it out through reasoning. In the structured binding version,
that would likely go away and I'd probably argue harder for this being an
unfortunate design for maintainers and reviewers.
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
