hokein added inline comments.
================ Comment at: clang/lib/Parse/ParseExprCXX.cpp:3109 + QualType PreferredType; + if (TypeRep) + PreferredType = Actions.ProduceConstructorSignatureHelp( ---------------- sammccall wrote: > 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." > 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). I believe the issue exists even before this patch, it was just not caught by tests. Added one. ================ Comment at: clang/lib/Sema/SemaType.cpp:1594 } + if (Result->containsErrors()) + declarator.setInvalidType(true); ---------------- sammccall wrote: > are you sure you want this in the individual cases, rather than once at the > end of this function? ah, good point, moved the end of the `switch` statement. 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
