erichkeane added a comment. In D146971#4224519 <https://reviews.llvm.org/D146971#4224519>, @ilya-biryukov wrote:
> In D146971#4224465 <https://reviews.llvm.org/D146971#4224465>, @erichkeane > wrote: > >> One other thing we probably should do is have an assert when creating a >> function type that none of its params are null. WDYT? > > This would definitely be great, however I don't think this is possible > without some large redesign. > Currently, `FunctionType` does not store parameter decls. Instead, they are > stored in the `FunctionTypeLoc`. > The latter gets created by allocating > <https://github.com/llvm/llvm-project/blob/5f34259609f604bfcd5cbf324a32d265e6a5d347/clang/lib/AST/ASTContext.cpp#L3087> > the necessary number of bytes and filling the empty state. > In principle, it would be much easier if `FuncionTypeLoc` and `FunctionType` > were created in parallel, ensuring consistency there would be much easier. I > have also though about this, but this looks like a large change and I didn't > explore further. 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. ================ 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: > > 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. 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
