[PATCH] D137020: [clang][AST] Handle variable declaration with unknown typedef in C

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:177-178
+  case tok::kw___attribute:
+  case tok::kw_const:
+  case tok::kw_volatile:
+  case tok::star:
----------------
What about other qualifiers? `_Nullable` and `restrict` and whatnot?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:184-185
+  case tok::ampamp:
+  case tok::kw___declspec:
+  case tok::l_square:
+    return P.getLangOpts().CPlusPlus;
----------------
Why are these pinned to C++? `__declspec` is used in C and shows up in the same syntactic locations as in C++, and `[` seems like you're looking for `[[]]`-style attributes (perhaps?) but those also exist in C.

What about parens? e.g.,
```
_BitNit(12) foo; // Oops, meant to type _BitInt(12)
```




================
Comment at: clang/test/AST/ast-dump-invalid.c:8-11
+  unknown_t const *c;
+  // CHECK:      VarDecl{{.*}} invalid c 'const int *'
+  unknown_t volatile *d;
+  // CHECK:      VarDecl{{.*}} invalid d 'volatile int *'
----------------
I'd also like to see a test where the qualifier comes before the unknown type name:
```
const unknown_t e;
```
and for typos with multi-keyword types:
```
unsinged long g;
unsigned lnog h;
```
and for situations that are just utter nonsense rather than types:
```
unknown_t[12] i;
unknown_t&&& j;
```
ohhh.... and GNU statement expressions make this a bit interesting because that's a place where a declaration and an expression get a bit mixed up:
```
({ int val = 0, i = 12; vla * i; }) // Typo: vla should be val, but this is *not* a declaration, it's the expression determining the value of the statement expression.
({ int val = 0, i = 12; vla & i; }) // Similar but an interesting case for C++
```



================
Comment at: clang/test/Parser/recovery.c:109
+  unknown_t __attribute__ ((aligned (16))) f; // expected-error {{unknown type name 'unknown_t'}}
+  int g = unknown_t * a; // expected-error {{use of undeclared identifier 'unknown_t'}}
+}
----------------
There is a behavioral difference here that is plausibly a regression: we used to get `use of undeclared identifier 'a'` here as well but because we recover as-if `a` was declared as type `int`, that diagnostic has gone away. I think that's reasonable as the user was already told about the issue with the declaration of `a`.


================
Comment at: clang/test/Parser/recovery.c:105
+  unknown_t a; // expected-error {{unknown type name 'unknown_t'}}
+  unknown_t *b; // expected-error {{unknown type name 'unknown_t'}}
+  unknown_t const *c; // expected-error {{unknown type name 'unknown_t'}}
----------------
urazoff wrote:
> sammccall wrote:
> > this diagnostic is worse than the old one (less accurate).
> > 
> > (I think it's OK to trade off diagnostics quality if it's better on balance, maybe leave a comment?)
> Just to mention, this is exact behavior of clang for C++.
> this diagnostic is worse than the old one (less accurate).

Less accurate how? It went from `use of undeclared identifier 'unknown_t'` to `unknown type name 'unknown_t'` and that seems accurate to me given that the undeclared identifier is written in a type position.


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

https://reviews.llvm.org/D137020