aaron.ballman added a comment.
Thank you for your patience while WG14 finalized the feature and I wrapped my
head around the moving parts involved. I think this is shaping up nicely, but
there's still some diagnostic issues. The biggest ones are:
- We should have an extension warning for array use with string literals `auto
str[] = "testing";`
- We should be careful about testing underspecified declarations in a way that
give the impression they're an extension, so we should sprinkle more fixme
comments around the tests
- `_Atomic auto x = 12;` is now something we should be accepting and deduce as
an `_Atomic int`
I think there's a typo in the patch summary:
> auto in an compound statement
I think you mean "as the type of a compound literal"?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:240-242
+def ext_c2x_auto_pointer_declaration : Extension<
+ "explicit declaration of 'auto' pointers is a Clang extension">,
+ InGroup<DiagGroup<"auto-decl-extensions">>;
----------------
I think we'll need this diagnostic for more than just `*` in a declaration.
Consider a case like:
```
auto val = (struct X { int y; }){ 0 };
```
accepting this is also an extension because it declares `y`, which is not an
"ordinary" identifier. See 6.7.9p2-3
Also, because this is about the syntax used, I think this might be a diagnostic
that should live in DiagnosticParseKinds.td (but maybe it's correct here, I've
not reviewed enough yet to see).
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2417
def err_auto_init_list_from_c : Error<
- "cannot use __auto_type with initializer list in C">;
+ "cannot use %select{'auto'|'decltype(auto)'|'__auto_type'}0 with initializer
list in C">;
def err_auto_bitfield : Error<
----------------
I'm surprised to see `decltype(auto)` here as `decltype` isn't a keyword in C
so it'd be invalid syntax; let's switch it to something that makes it obvious
that we should never issue that particular part of the diagnostic in C.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:12684-12685
+ // Diagnose auto array declarations in C2x,
+ // unless it's a char array or an initialiser list which is diagnosed later
+ if (getLangOpts().C2x && Type->isArrayType() &&
----------------
Comment can be re-flowed to 80 columns and the comment should end in a period.
(Weirdly, Phabricator isn't letting me suggest edits just within this file.)
================
Comment at: clang/lib/Sema/SemaDecl.cpp:12687-12688
+ if (getLangOpts().C2x && Type->isArrayType() &&
+ !isa_and_nonnull<StringLiteral>(Init) &&
+ !isa_and_nonnull<InitListExpr>(Init)) {
+ Diag(Range.getBegin(), diag::err_auto_not_allowed)
----------------
This can switch to using `!isa_and_present<StringLiteral, InitListExpr>(Init)`
(the `_and_nonnull` variant is quietly deprecated).
================
Comment at: clang/lib/Sema/SemaDecl.cpp:12690
+ Diag(Range.getBegin(), diag::err_auto_not_allowed)
+ << (int)Deduced->getContainedAutoType()->getKeyword() << 23 << Range;
+ return QualType();
----------------
Please add a `/* */` comment before `23` detailing what that magic number will
be printing.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4780
+ // Make sure that we treat 'char[]' equaly as 'char*' in C2x mode
+ auto *String = dyn_cast<StringLiteral>(Init);
----------------
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4789-4792
+ // Emit a warning if 'auto*' is used in pedantic and in C2x mode
+ if (getLangOpts().C2x && Type.getType()->isPointerType()) {
+ Diag(Type.getBeginLoc(), diag::ext_c2x_auto_pointer_declaration);
+ }
----------------
I think this diagnostic should happen from the parser rather then when
determining the concrete type; that ensures consistent diagnosis of the syntax
regardless of whether we needed to deduce the type or not.
================
Comment at: clang/test/C/C2x/n3007.c:6-22
+void test_qualifiers(int x, const int y) {
+ const auto a = x;
+ auto b = y;
+ static auto c = 1UL;
+ int* pa = &a; // expected-warning {{initializing 'int *' with an expression
of type 'const int *' discards qualifiers}}
+ const int* pb = &b;
+ int* pc = &c; // expected-warning {{incompatible pointer types initializing
'int *' with an expression of type 'unsigned long *'}}
----------------
Adding one more test case for `restrict`-qualified pointers.
================
Comment at: clang/test/C/C2x/n3007.c:25
+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}} \
----------------
What we finally landed on in WG14 is that this should be accepted. In this
form, `_Atomic` is being used as a type qualifier (not a type specifier), and
6.7.9p2 explicitly talks about qualifiers: "The inferred type of the declared
object is the type of the assignment expression after lvalue, array to pointer
or function to pointer conversion, additionally qualified by qualifiers and
amended by attributes as they appear in the declaration specifiers, if any."
So this should form an `_Atomic int` initialized to the value `12`, and that
matches GCC's behavior: https://godbolt.org/z/s468s86z6
================
Comment at: clang/test/C/C2x/n3007.c:32
+
+ _Static_assert(_Generic(&i, _Atomic auto *: 1)); // expected-error {{_Atomic
cannot be applied to type 'auto' which is not trivially copyable}} \
+ expected-error {{'auto'
not allowed here}}
----------------
The first error on this line isn't helpful; the second error is the only one
I'd expect to see here.
================
Comment at: clang/test/C/C2x/n3007.c:63
+void test_sizeof_alignas(void) {
+ auto auto_size = sizeof(auto); // expected-error {{expected expression}}
+ _Alignas(auto) int b[4]; // expected-error {{expected expression}}
----------------
To make it clear that use of `auto` within `sizeof` is the issue, not the
initialization itself.
================
Comment at: clang/test/C/C2x/n3007.c:81-84
+ // FIXME: This should be diagnosed as invalid as described in N3006
+ auto p1 = (struct { int a; } *)0;
+ struct s;
+ auto p2 = (struct s { int a; } *)0;
----------------
Might need to re-flow the comment as well.
================
Comment at: clang/test/C/C2x/n3007.c:102
+ auto test_char_ptr = "test";
+ auto test_char_ptr2[] = "another test";
+ auto auto_size = sizeof(auto); // expected-error {{expected expression}}
----------------
This should also get the "is a clang extension" warning because the declarators
include `[]`.
================
Comment at: clang/test/C/C2x/n3007.c:116
+
+void test_compound_literral(void) {
+ auto a = (int){};
----------------
================
Comment at: clang/test/CodeGen/auto.c:15
+void misc_declarations(void) {
+ auto strct_ptr = (struct { int a; } *)0; // CHECK: %strct_ptr = alloca ptr,
align 8
+ auto int_cl = (int){13}; // CHECK: %int_cl = alloca i32,
align 4
----------------
================
Comment at: clang/test/CodeGen/auto.c:19-22
+ auto se = ({ // CHECK: %se = alloca i32, align 4
+ auto snb = 12; // CHECK: %snb = alloca i32, align 4
+ snb;
+ });
----------------
I'd like to see a sema test for this:
```
auto t = ({
auto b = 12;
b;
)};
_Generic(t, int : 1);
```
================
Comment at: clang/test/Parser/c2x-auto.c:71-72
+
+ auto s_int = (struct { int a; } *)0; // c17-error {{incompatible pointer to
integer conversion initializing 'int' with an expression of type}} \
+ c17-error {{type specifier missing,
defaults to 'int'; ISO C99 and later do not support implicit int}}
+
----------------
================
Comment at: clang/test/Parser/c2x-auto.c:97
+
+void test_function_pointers(void) {
+ extern auto auto_ret_func(void); // c2x-error {{'auto' not allowed in
function return type}} \
----------------
================
Comment at: clang/test/Parser/c2x-auto.c:122
+
+ _Atomic auto atom2 = 12; // c2x-error {{_Atomic cannot be applied to type
'auto' which is not trivially copyable}} \
+ c17-error {{type specifier missing, defaults to
'int'; ISO C99 and later do not support implicit int}}
----------------
This one should be fixed and accepted.
================
Comment at: clang/test/Parser/c2x-auto.c:124
+ c17-error {{type specifier missing, defaults to
'int'; ISO C99 and later do not support implicit int}}
+}
----------------
I'd like to see parsing tests for:
```
auto ident [[clang::annotate("this works")]] = 12; // No diagnostic expected
```
We should also add a test that this C++'ism doesn't work in C:
```
int a = auto(0);
```
================
Comment at: clang/test/Sema/c2x-auto.c:74
+ auto str = "this is a string";
+ auto str2[] = "this is a string";
+ auto (str3) = "this is a string";
----------------
This should also get an extension warning.
================
Comment at: clang/test/Sema/c2x-auto.c:119
+ return x;
+}
----------------
Some additional test cases to consider:
```
_Complex auto i = 12.0; // Should be rejected because _Complex is a type
specifier; however,
// when auto becomes a type specifier, this should be
accepted. GCC
// rejects but if we end up accepting, I won't be sad
-- we'll need an
// extension warning in that case though.
void foo(void) {
extern void foo(int a, int array[({ auto x = 12; x;})]); // This is a use of
`auto` within function prototype
// scope, but should
still be accepted.
}
```
================
Comment at: clang/www/c_status.html:1193
<td><a
href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3006.htm";>N3006</a></td>
- <td class="unknown" align="center">Unknown</td>
+ <td class="none" align="center">Unknown</td>
</tr>
----------------
This can be committed as an NFC change separate from this review, given that
it's slightly orthogonal.
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
