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

Mon Nov 7 06:36:36 PST 2022

sammccall added a comment.

I admire the goals of this patch, but I think it needs some explanation "why we expect this to be roughly correct/safe", rather than just trying to think of cases that might go wrong and patch them.

The test coverage for incorrect code is not good, I'd expect to need to add some new cases here. (And if there are benefits to the AST, we'd want to test those).

================
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;
----------------
urazoff wrote:
> aaron.ballman wrote:
> > 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!).
> There is weird behavior in case of ObjC range-based for loop. For example, in `for (NSString* key in xyz)`  token for `in` keyword is of type `Identifier` by the call of `Parser::isDeclarationSpecifier`. So I decided to exempt it in first version and discuss it in review.
This looks very suspicious to me, for example in `a * b;`. isDeclarationSpecifier() is going to return true when pointing at `a`. In fact `a` may be either a type or an expression here.

Even if this right now this function never gets called for that case, it's not obvious why it wouldn't be in the future, or wouldn't be called for similar cases.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137020