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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
