a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.
Hi Tom,
Thanks for the fixes! The patch looks good to me now. I have only a small nit
inline.
================
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:
> 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.
================
Comment at: unittests/AST/ASTImporterTest.cpp:581
+ // Don't try to match the template contents if template parsing is delayed.
+ if (std::find(std::begin(Args), std::end(Args),
+ "-fdelayed-template-parsing") != Args.end()) {
----------------
LLVm has a set of nice helpers for working with containers in range -like
style. I'd recommend you to use llvm::find here:
`if (llvm::find(Args), "-fdelayed-template-parsing") != Args.end()) { `
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
