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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: clang-language-wg, aaron.ballman.
aaron.ballman added a comment.

Thank you for the changes! One thing you should add is a release note so users know there's been a diagnostic improvement.



================
Comment at: clang/lib/Parse/ParseDecl.cpp:5384-5385
 
+bool Parser::IsUnknownTypedefName() {
+  switch (NextToken().getKind()) {
+  default:
----------------
This is a pretty unsafe function to call from arbitrary parsing contexts. We should be asserting that the parser is in a reasonable state to even ask this question; alternatively, we could make this a static function that accepts a `Parser&` object.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5388
+    return false;
+  case tok::kw___attribute:
+  case tok::star:
----------------
Why only `__attribute__` and not `__declspec`? `[[]]` attributes as well?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5392-5394
+  case tok::amp:
+  case tok::ampamp:
+    return getLangOpts().CPlusPlus;
----------------
Looking for `*`, `&`, and `&&` will help catch some cases... but it's not really going to help for situations like `unknown const *` or other qualifiers...


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5425
+    // node in AST for such cases which is good for AST readers.
+    if (IsUnknownTypedefName() && !getLangOpts().ObjC)
+      return true;
----------------
Why is ObjC exempted from this?

I need to think about this a whole lot more. On the one hand, I like the changes happening in the test cases; those look like good changes to me. But on the other hand, this function is called in a fair number of places to make parsing decisions and I'm not convinced that speculative answers are the best way to go from this interface. I need to see if I can find any situations where this is called and we'd get worse parsing behavior (or incorrect parsing behavior!).


================
Comment at: clang/test/SemaOpenCL/invalid-pipes-cl1.2.cl:29-34
  reserve_id_t r;
 #if defined(__OPENCL_C_VERSION__)
-// expected-error at -2 {{use of undeclared identifier 'reserve_id_t'}}
+// expected-error at -2 {{unknown type name 'reserve_id_t'}}
 #else
 // expected-error at -4 {{unknown type name 'reserve_id_t'}}
 #endif
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137020