sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang/lib/Parse/ParseExprCXX.cpp:3109 + QualType PreferredType; + if (TypeRep) + PreferredType = Actions.ProduceConstructorSignatureHelp( ---------------- hokein wrote: > sammccall wrote: > > Add a comment for what the null case means? (When do we actually hit this?) > yeah, the > [`CompleteTest`](https://github.com/llvm/llvm-project/blob/master/clang/unittests/Sema/CodeCompleteTest.cpp#L490) > hits the assertion after this patch. > > the assertion seems incorrect -- IIUC, the assertion is for the > `isInvalidType()` sanity check on Line 3090, however > In `ActOnTypeName`, `DeclaratorInfo` could be modified (by > `GetTypeForDeclarator`) before calling `isInvalidType`. > > > btw, I think for the CodeCompleteTest, would be nicer to make `ActOnTypeName` > return `decltype(<recovery-expr>(bar))`, rather than the null type, but I'm > not sure changing the `ActOnTypeName` behavior has any side effect. > the assertion seems incorrect -- IIUC, the assertion is for the > isInvalidType() sanity check on Line 3090, however What you say makes sense, but I think it's worth probing why it's not currently hit (e.g. by `int x(auto);`, where `GetDeclSpecTypeForDeclarator` marks the decl as invalid because auto isn't allowed in a prototype). > btw, I think for the CodeCompleteTest, would be nicer to make ActOnTypeName > return decltype(<recovery-expr>(bar)), rather than the null type Definitely. I think "invalid" on a type-concept is stronger than what we're looking for - since we're not tracking errors in decls, we'd want to use "haserrors" on type-concepts and then promote to "invalid" on decl-concepts. Ugh, the design of "Declarator" makes this difficult, because there's no distinction between "type of this declarator is invalid" and "type of this declarator makes the declarator invalid". I'd suggest leaving a FIXME on the changes in SemaType, saying something like "we want resulting declarations to be marked invalid, but claiming the type is invalid is too strong - e.g. it causes ActOnTypeName to return a null type." ================ Comment at: clang/lib/Sema/SemaType.cpp:1594 } + if (Result->containsErrors()) + declarator.setInvalidType(true); ---------------- are you sure you want this in the individual cases, rather than once at the end of this function? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77037/new/ https://reviews.llvm.org/D77037 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
