[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Aaron Ballman via Phabricator via cfe-commits]

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