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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

Comments on tests.



================
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:
> 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.


================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp:6-28
+  static_assert(__is_same(decltype(auto(v)), int *));
+  static_assert(__is_same(decltype(auto("lit")), char const *));
+
+  int fn(char *);
+  static_assert(__is_same(decltype(auto(fn)), int (*)(char *)));
+
+  constexpr long i = 1;
----------------
It is definitely a good idea to repeat all of these tests with braces too: `auto{v}`, `auto{"lit"}`, etc.


================
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
+  }
----------------
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//?


================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp:56
+  A(const A &);
+};
----------------
I'm not sure which test file it should go in, but you definitely want a test somewhere proving that `auto` doesn't copy prvalues. E.g.
```
struct Uncopyable {
    explicit Uncopyable(int);
    Uncopyable(Uncopyable&&) = delete;
};
Uncopyable u = auto(Uncopyable(auto(Uncopyable(42))));
```


================
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));
----------------
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).


================
Comment at: clang/test/CXX/expr/expr.post/expr.type.conv/p1-2b.cpp:25
+  foo(auto {a, 3.14});  // expected-error {{initializer for functional-style cast to 'auto' contains multiple expressions}}
+  foo(auto({a, 3.14})); // expected-error {{initializer for functional-style cast to 'auto' contains multiple expressions}}
+}
----------------
I'd also check `auto{1,2}` (ill-formed, does //not// deduce `initializer_list<int>`) and `auto{{1,2}}` (also ill-formed).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113393/new/

https://reviews.llvm.org/D113393