[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

Tue Mar 28 06:03:00 PDT 2023

erichkeane accepted this revision.
erichkeane added inline comments.
This revision is now accepted and ready to land.

================
Comment at: clang/lib/Sema/SemaType.cpp:5949
   assert(!T.isNull() && "T must not be null at the end of this function");
-  if (D.isInvalidType())
+  if (!AreDeclaratorChunksValid)
     return Context.getTrivialTypeSourceInfo(T);
----------------
ilya-biryukov wrote:
> erichkeane wrote:
> > ilya-biryukov wrote:
> > > erichkeane wrote:
> > > > Shouldn't the `D.setInvalidType(true)` in each of the branches work here?  So this variable is unnecessary?  Else this is a good change IMO. 
> > > (Let me know if I'm misreading the suggestion, I was not sure if my understanding is correct).
> > > If we call `setInvalidType` more, we would actually get more crashes as the problematic branch is the one that calls `getTrivialTypeSourceInfo`.
> > > To avoid the extra variable, we could instead ensure that the type always gets replaced with something trivial `T = Context.IntTy`. But I didn't want to go this path because this causes worse error recovery (going from something like `void(<recovered-to:int>)` to `<recovered-to:int>`) and possibly correctness issues (e.g. will we start getting function-decls or lambdas that do not have function types and other assertions may fire).
> > My suggestion was that `setInvalidType` is called everywhere that `AreDeclatorChunksValid` above (sans 1 perhaps?). So my question was whether this introduced variable was necessary.
> Ah, thanks for clarifying the question.
> `setInvalidType` is called in much more places and the point of this patch is to capture a subcategory of invalid types that still follow the declarator structure.
> 
Got it, thanks for the clarification.  I'm OK with this as-is, then.  

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146971