[PATCH] D113393: [c++2b] Implement P0849R8 auto(x)

[Zhihao Yuan via Phabricator via cfe-commits]

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