[PATCH] D133289: [C2X] N3007 Type inference for object definitions
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue May 9 10:43:32 PDT 2023
aaron.ballman added a comment.
Thank you for the continued work on this! In general, I think the test coverage should verify the deduced type information more often. e.g., if we accept the code, there should probably be a `static_assert` nearby that ensures the deduced type is what we'd expect.
I applied the patch locally and tried out various things, and here's some test coverage I think we're lacking.
// Important: these two declarations are at file scope, we're testing that the storage
// class specifier is ignored because we're deducing the type.
auto i = 12; // Ok, deduces int
auto j = 12ull; // Ok, deduces unsigned long long
// This is testing whether storage specifier order matters:
void func() {
const auto static i = 12; // Ok, static variable of type const int
}
In D133289#4279054 <https://reviews.llvm.org/D133289#4279054>, @to268 wrote:
> I have *hopefully* fixed my broken patch.
> This is all the things in need to address:
>
> auto str2[] = "string";
> [[clang::overloadable]] auto test(auto x) { return x; }
>
> and maybe retry to support `auto` in compound literals,
> becaue i have seen heavy macro users that want compound literals to work.
>
> I think it's safe to say that we will not support all features from N3076 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3076.pdf> in this patch.
My testing of your patch suggests those aren't issues left to address (perhaps). e.g.,
auto test[] = "foo";
static_assert(_Generic(test, char * : 1, default : 0));
this matches what I'd expect (no warning, no failed assertion; string literals have type `char []` in C, not `const char[]`.
My testing of `__attribute__((overloadable))` with your patch is that we currently reject use of `auto` in those functions. I think that's good initial behavior -- if we want to allow `auto` there, that's probably better left to a follow-up patch anyway because I'd expect there to be some debate around whether that's a good idea or not.
As for compound literals, I think we've determined they're not supported by C currently.
================
Comment at: clang/test/C/C2x/n3007.c:20
+void test_atomic(void) {
+ _Atomic auto i = 12; // expected-error {{_Atomic cannot be applied to type 'auto' which is not trivially copyable}}
+ _Atomic(auto) j = 12; // expected-error {{'auto' not allowed here}} \
----------------
This diagnostic is incorrect -- I believe this code should be accepted.
================
Comment at: clang/test/C/C2x/n3007.c:21
+ _Atomic auto i = 12; // expected-error {{_Atomic cannot be applied to type 'auto' which is not trivially copyable}}
+ _Atomic(auto) j = 12; // expected-error {{'auto' not allowed here}} \
+ expected-error {{a type specifier is required for all declarations}}
----------------
This diagnostic is correct.
================
Comment at: clang/test/C/C2x/n3007.c:31-32
+ double A[3] = { 0 };
+ auto pA = A;
+ auto qA = &A;
+ auto pi = 3.14;
----------------
We should test that the types match our expectations:
```
double A[3] = { 0 };
auto pA = A; // pA is double * due to array decay on A
static_assert(_Generic(pA, double * : 1, default : 0));
auto qA = &A; // qA is double (*)[3]
static_assert(_Generic(qA, double (*)[3] : 1, default : 0));
```
================
Comment at: clang/test/C/C2x/n3007.c:46
+ auto auto_size = sizeof(auto); // expected-error {{expected expression}}
+ _Alignas(auto) auto b[4]; // expected-error {{declaration of variable 'b' with deduced type 'auto[4]' requires an initializer}} \
+ expected-error {{expected expression}}
----------------
I think this would make it more clear that what's being tested is the `_Alignas` behavior.
================
Comment at: clang/test/C/C2x/n3007.c:56-57
+void test_initializer_list(void) {
+ // FIXME: as mentioned in N3076, it seems that intializer list are allowed
+ // even with an initializer list containing one item
+ auto a = {}; // expected-error {{cannot use 'auto' with initializer list in C}}
----------------
This should now be resolved in N3096 so the FIXME comment can be removed.
================
Comment at: clang/test/C/C2x/n3007.c:66
+void test_structs(void) {
+ auto a = (struct { int a; } *)0;
+ struct B { auto b; }; // expected-error {{'auto' not allowed in struct member}}
----------------
This is awfully close to the examples from the paper (6.7.9p3):
```
auto p1 = (struct { int a; } *)0; // error expected
struct s;
auto p2 = (struct s { int a; } *)0; // error expected
```
They are expected to be diagnosed due to changes in N3006 (underspecified object definitions), so I think it's fine to add a FIXME comment with the test cases instead of trying to also implement N3006 at the same time. We currently list that as `unknown` in our status page, but I think it's clearly a `no` now.
================
Comment at: clang/test/C/C2x/n3007.c:100
+ auto d = (int){ 0, 1 }; // expected-warning {{excess elements in scalar initializer}}
+ // FIXME: This should be accepted per C2x 6.5.2.5p4
+ auto auto_cl = (auto){13}; // expected-error {{expected expression}}
----------------
No longer accurate (see comments below), so I think this FIXME comment can be removed.
================
Comment at: clang/test/C/C2x/n3007.c:106-107
+ int a;
+ auto *ptr = &a;
+ auto *ptr2 = a; // expected-error {{variable 'ptr2' with type 'auto *' has incompatible initializer of type 'int'}}
+ auto nptr = nullptr;
----------------
I'd like to see a test that passes `-pedantic` which diagnoses these declarations with a warning about accepting this code being a Clang extension. (The standard requires we support `auto ident = expr;` but leaves other declarator pieces up to the implementation.)
Note, 6.7.9p2 made this implementation-defined behavior, so we also need to add some documentation to meet that requirement.
================
Comment at: clang/test/Parser/c2x-auto.c:46-51
+ auto b[4]; // c2x-error {{declaration of variable 'b' with deduced type 'auto[4]' requires an initializer}} \
+ c17-error {{type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int}}
+
+ auto array[auto]; // expected-error {{expected expression}} \
+ c2x-error {{declaration of variable 'array' with deduced type 'auto' requires an initializer}} \
+ c17-error {{type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int}}
----------------
The C2x diagnostics here are kind of mean because if you give an initializer, we then reject the code anyway. If possible, it'd be nice to reject this for use of `auto` with an array declarator rather than the lack of an initializer.
================
Comment at: clang/test/Parser/c2x-auto.c:78-81
+
+ _Alignas(4) auto b[4]; // c2x-error {{declaration of variable 'b' with deduced type 'auto[4]' requires an initializer}} \
+ c17-error {{type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int}}
+}
----------------
This is tested below with a more clear test.
================
Comment at: clang/test/Sema/c2x-auto.c:26-27
+
+ // FIXME: This should be accepted per C2x 6.5.2.5p4
+ auto auto_cl = (auto){13}; // expected-error {{expected expression}}
+}
----------------
This FIXME is no longer accurate, I think it's not clear whether we should accept or reject this code from reading the standard, but I'm now thinking we should reject it for two reasons: 1) 6.7.9p1 says "A declaration for which the type is inferred shall contain the storage-class specifier auto", but a compound literal is not a declaration, it's an expression. What's more, it doesn't have a direct declarator or an equals sign as specified in p2. 2) GCC rejects: https://godbolt.org/z/K8sqr9qTT
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133289/new/
https://reviews.llvm.org/D133289
More information about the cfe-commits
mailing list