lichray marked 10 inline comments as done. lichray added inline comments.
================ Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1032 + // the typename-specifier in a function-style cast expression may + // be 'auto' since C++2b Diag(Tok.getLocation(), ---------------- rsmith wrote: > Quuxplusone wrote: > > rsmith wrote: > > > Nice catch :) > > I see zero hits for `git grep 'decltype[(]auto[^)] clang/`. So it seems > > this corner of the grammar has been missing any tests for a long time, but > > I hope this PR will add some. > > ``` > > decltype(auto*) i = 42; // should be a syntax error > > decltype(auto(42)) i = 42; // should be OK > > decltype(auto()) i = 42; // should be a syntax error > > ``` > > Right now I don't see any tests for this stuff in this PR either. So it > > needs some. > Some of this is tested in the new file > test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp (you might > need to expand it manually; full-page search for `decltype(auto(` might > otherwise not find it). It looks like we are missing tests for things like > `decltype(auto*)`. It's not obvious to me what diagnostic would be most > useful if `decltype(auto` is not followed by `)`, `(`, or `{`, but my guess > is that "expected `)`" pointing at the `*`, rather than an error on the > `auto` token, would be the least surprising. So maybe this should be > ``` > if (Tok.is(tok::kw_auto) && NextToken().isNot(tok::l_paren, tok::l_brace)) { > ``` > ? The r0 patch makes `decltype(auto *)` diagnose in the same way `decltype(long *)` do; looks good enough to me. ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:1482-1486 + if (Exprs.size() == 1) { + if (auto p = dyn_cast_or_null<InitListExpr>(Exprs[0])) { + Inits = MultiExprArg(p->getInits(), p->getNumInits()); + } + } ---------------- rsmith wrote: > You should only do this if `ListInitialization` is `true`. Otherwise we'll > accept the invalid syntax `auto({a})`. My bad. Deemed `new auto({a})` as valid; now corrected. ================ Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp:51 + f(A(*this)); // ok + f(auto(*this)); // ok in P0849 + } ---------------- Quuxplusone wrote: > Should you check that `__is_same(decltype(auto(*this)), A)`? > Should you check that `__is_same(decltype(auto(this)), A*)`? > Should you test inside a const member function too? > Should you check that the friend function actually can call > `auto(some_a_object)`, the same way it can call `A(some_a_object)` — and more > to the point, that a non-friend //can't//? All good ideas. ================ Comment at: clang/test/CXX/expr/expr.post/expr.type.conv/p1-2b.cpp:17-18 + + foo(auto(a)); + foo(auto {a}); + foo(auto(a)); ---------------- Quuxplusone wrote: > What is the significance of the whitespace here? Is this a lexer test? > Normally it'd be just `auto(a)`, `auto{a}` — just like `T(a)`, `T{a}`. I > think we should follow convention (again, unless this is a lexer test — in > which case we should probably test //both// syntaxes with and without > whitespace). Blame clang-format :/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113393/new/ https://reviews.llvm.org/D113393 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits