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

[Guillot Tony via Phabricator via cfe-commits]

to268 added a comment.

Here are all the details that i've said earlier



================
Comment at: clang/lib/Parse/ParseExpr.cpp:1515
+    // 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);
----------------
aaron.ballman wrote:
> Why would this not be handled from `Sema::ActOnCompoundLiteral()`?
You're right, it's better to handle this in `Sema::ActOnCompoundLiteral()`
The problem is that right now, the usage of the `auto` keyword in a compound literal isn't parsed as a compound literal. 
This compound literal is unable to reach `Sema::ActOnCompoundLiteral()` because the parser emits that it's an invalid expression.

To summarize, i'm unable to handle handle a compound literal that uses the `auto` keyword inside `Sema::ActOnCompoundLiteral()`
if it's never going to be called.
```
int test_ncl = (int){12};           // Parsed as a CL
auto test_cl = (auto){12};          // Not parsed as a CL and emits "expected expression"
/*
 * |-DeclStmt 0x562180ede8b0 <line:17:5, col:29>
 * | `-VarDecl 0x562180ede730 <col:5, col:28> col:9 test_ncl 'int' cinit
 * |   `-ImplicitCastExpr 0x562180ede898 <col:20, col:28> 'int' <LValueToRValue>
 * |     `-CompoundLiteralExpr 0x562180ede870 <col:20, col:28> 'int' lvalue
 * |       `-InitListExpr 0x562180ede828 <col:25, col:28> 'int'
 * |         `-IntegerLiteral 0x562180ede7b0 <col:26> 'int' 12
 * |-DeclStmt 0x562180ede970 <line:18:5, col:30>
 * | `-VarDecl 0x562180ede908 <col:5, col:10> col:10 invalid test_cl 'auto'
 * `-ReturnStmt 0x562180ede9a8 <line:19:5, col:12>
 *   `-IntegerLiteral 0x562180ede988 <col:12> 'int' 0
 */
```


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1515
+    // 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);
----------------
to268 wrote:
> aaron.ballman wrote:
> > Why would this not be handled from `Sema::ActOnCompoundLiteral()`?
> You're right, it's better to handle this in `Sema::ActOnCompoundLiteral()`
> The problem is that right now, the usage of the `auto` keyword in a compound literal isn't parsed as a compound literal. 
> This compound literal is unable to reach `Sema::ActOnCompoundLiteral()` because the parser emits that it's an invalid expression.
> 
> To summarize, i'm unable to handle handle a compound literal that uses the `auto` keyword inside `Sema::ActOnCompoundLiteral()`
> if it's never going to be called.
> ```
> int test_ncl = (int){12};           // Parsed as a CL
> auto test_cl = (auto){12};          // Not parsed as a CL and emits "expected expression"
> /*
>  * |-DeclStmt 0x562180ede8b0 <line:17:5, col:29>
>  * | `-VarDecl 0x562180ede730 <col:5, col:28> col:9 test_ncl 'int' cinit
>  * |   `-ImplicitCastExpr 0x562180ede898 <col:20, col:28> 'int' <LValueToRValue>
>  * |     `-CompoundLiteralExpr 0x562180ede870 <col:20, col:28> 'int' lvalue
>  * |       `-InitListExpr 0x562180ede828 <col:25, col:28> 'int'
>  * |         `-IntegerLiteral 0x562180ede7b0 <col:26> 'int' 12
>  * |-DeclStmt 0x562180ede970 <line:18:5, col:30>
>  * | `-VarDecl 0x562180ede908 <col:5, col:10> col:10 invalid test_cl 'auto'
>  * `-ReturnStmt 0x562180ede9a8 <line:19:5, col:12>
>  *   `-IntegerLiteral 0x562180ede988 <col:12> 'int' 0
>  */
> ```
When we are parsing an expression between parentheses in `Parser::ParseParenExpression()` line 2972,
we are calling `Parser::isTypeIdInParens()` which when using C will return `true` if it's a type specifier,
which is not the case for the `auto` keyword.

Ultimately, we fall into the conditional block of `Parser::ParseParenExpression()` line 3191,
which will return an `ExprError()` (AKA: a parsing error).

This is why we are unable to forbid a compound literal using the `auto` keyword at the 
Semantic Analysis stage and why this feature is not working out of the box in the first place.


================
Comment at: clang/test/C/C2x/n3007.c:7
+void test_qualifiers(int x, const int y) {
+  // TODO: prohibit cont auto
+  const auto a = x;
----------------
aaron.ballman wrote:
> Why? My reading of the grammar is that `const auto` is valid. `const` is a type-specifier-qualifier declaration-specifier, and `auto` is a storage-class-specifier declaration-specifier, and a declaration is allowed to use a sequence of declaration-specifiers.
It's a mistake, i don't remember why i've added this comment in the first place and i've forgot his existence


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133289