[PATCH] D133289: [C2X] N3007 Type inference for object definitions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the update to this!



================
Comment at: clang/lib/Parse/ParseExpr.cpp:1526
+    // This is a temporary fix while we don't support C2x 6.5.2.5p4
+    if (getLangOpts().C2x && GetLookAheadToken(2).getKind() == tok::l_brace) {
+      Diag(Tok, diag::err_c2x_auto_compound_literal_not_allowed);
----------------
I don't think this is quite right; I suspect we're going to need more complicated logic here. Consider a case like: `(auto const){ 12 }` or `(auto *){ nullptr }` where two tokens ahead is `)` and not `{`. (Note, these type specifications can be pretty arbitrarily complex.)

Given that this is a temporary workaround for a lack of support for storage class specifiers in compound literals, perhaps another approach is to not try to catch uses in compound literals. Instead, we could add tests with FIXME comments to demonstrate the behavior we get with compound literals now, and when we do that storage class specifier work, we will (hopefully) break those tests and come back to make them correct.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1530-1531
+    }
+  }
+    [[fallthrough]];
+
----------------
This looks a bit less visually jarring to me (the indentation differences were distracting). WDYT?


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1362
   // type specifier.
   if (S.getLangOpts().CPlusPlus &&
       TypeSpecType == TST_unspecified && StorageClassSpec == SCS_auto) {
----------------
to268 wrote:
> Do we need to include C2x just in case?
I don't think so -- we're in a language where the type specifier *is* optional for C2x.


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1370-1372
+  // specifier in a pre-C++11 dialect of C++
+  if ((!S.getLangOpts().CPlusPlus11 && !S.getLangOpts().C2x) &&
+      TypeSpecType == TST_auto)
----------------



================
Comment at: clang/test/C/C2x/n3007.c:13
+  int* pc = &c; // expected-warning {{incompatible pointer types initializing 'int *' with an expression of type 'unsigned long *'}}
+
+  const int ci = 12;
----------------
I'd also like a test for:
```
_Atomic auto i = 12;
_Atomic(auto) j = 12;

_Atomic(int) foo(void);
auto k = foo(); // Do we get _Atomic(int) or just int?
```


================
Comment at: clang/test/C/C2x/n3007.c:36
+  auto auto_size = sizeof(auto);  // expected-error {{expected expression}}
+  _Alignas(4) auto b[4];          // expected-error {{'b' declared as array of 'auto'}}
+}
----------------
I think this test should be `_Alignas(auto)` right?


================
Comment at: clang/test/C/C2x/n3007.c:40-41
+void test_arrary(void) {
+  auto a[4];          // expected-error {{'a' declared as array of 'auto'}}
+  auto b[] = {1, 2};  // expected-error {{'b' declared as array of 'auto'}}
+}
----------------
I think other array cases we want to test are:
```
auto a = { 1 , 2 }; // Error
auto b = { 1, }; // OK
auto c = (int [3]){ 1, 2, 3 }; // OK
```


================
Comment at: clang/test/C/C2x/n3007.c:56
+
+  _Static_assert(_Generic(test_char_ptr, const char * : 1, char * : 2) == 2, "C is weird");
+}
----------------
I'd also like to ensure we reject:
```
typedef auto (*fp)(void);
typedef void (*fp)(auto);

_Generic(0, auto : 1);
```
and we should ensure we properly get integer promotions:
```
short s;
auto a = s;
_Generic(a, int : 1);
```
and a test for use of an explicit pointer declarator (which is UB that we can define if we want to):
```
int a;
auto *ptr = &a; // Okay?
auto *ptr = a; // Error
```


================
Comment at: clang/test/Sema/c2x-auto.c:27
+  auto auto_cl = (auto){13};  // expected-error {{'auto' is not allowed in a compound literal}}
+  auto array[] = { 1, 2, 3 }; // expected-error {{'array' declared as array of 'auto'}}
+}
----------------
This one should not be accepted -- the grammar productions for the initialization allow for `{0}` and `{0,}` but no more than one element in the braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133289