ilya-biryukov added a comment.
In D146971#4224540 <https://reviews.llvm.org/D146971#4224540>, @erichkeane
wrote:
> Ah, hrmph. I guess I was just hoping for some assert (perhaps even in
> `GetFullTypeForDeclarator`?) to assert in order to give future folks a hint
> as to why their change crashes? Basically, whenever the
> `FunctionType`/`FunctionTypeLoc` are 'finalized' as a check perhaps? Not
> really looking for a refactor, so much as an assert to prevent us from
> getting here again.
Yeah, that makes a lot of sense. We need to fix the `FIXMEs` for that to work,
I'm eager to try this in follow-ups. But probably not in this particular change
as it seems like a lot of work and this is already moving in the right
direction.
We probably need asserts in two places: `GetFullTypeForDeclarator` and
`getTrivialTypeSourceInfo`.
I'm less worried about the former, but fixing `getTrivialTypeSourceInfo` might
be harder as it has a lot of call sites and I suspect some of them actually
pass `FunctionType` as inputs. The assert would basically prohibit creating
`FunctionTypeLoc` without parameter declarations.
I don't think it's unreasonable, the code that produces `FunctionTypeLoc`
without having parameters should either find the corresponding parameters or be
rewritten to use `QualType` instead of `TypeLoc`, which does not need
parameters.
But I expect this to be a lot of work to actually update the callsites, and
this will probably take quite some time.
================
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);
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146971/new/
https://reviews.llvm.org/D146971
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
