tahonermann accepted this revision as: tahonermann. tahonermann added a comment. This revision is now accepted and ready to land.
Looks good to me. ================ Comment at: clang/lib/Sema/SemaConcept.cpp:377-380 + SubstitutedExpression = ImplicitCastExpr::Create( + S.Context, SubstitutedExpression.get()->getType(), + CK_LValueToRValue, SubstitutedExpression.get(), + /*BasePath=*/nullptr, VK_PRValue, FPOptionsOverride()); ---------------- erichkeane wrote: > tahonermann wrote: > > erichkeane wrote: > > > tahonermann wrote: > > > > `ImplicitCastExpr::Create()` has the following assertion. I wonder if > > > > we need to guard against this here. > > > > clang/lib/AST/Expr.cpp: > > > > 2073 assert((Kind != CK_LValueToRValue || > > > > 2074 !(T->isNullPtrType() || T->getAsCXXRecordDecl())) && > > > > 2075 "invalid type for lvalue-to-rvalue conversion"); > > > > > > > > Or perhaps it would be better to call `Sema::DefaultLvalueConversion()` > > > > here? > > > I don't think we want all the baggage that DefaultLvalueConversion gets > > > us, including diagnostics/etc. That assert IS concerning though... I > > > presume we can come up with something to hit that. > > > > > > It DOES look like that DefeaultLValue conversion replaces the > > > nullptrtypes with a NullToPointer cast, so perhaps I should do that, and > > > doesn't do anything with a record type. > > I'm surprised the additional checks for `nullptr` aren't tripping that > > assert; it looks to me like it should. Any ideas? > I moved the creation of this AFTER 'CheckConstraintExpression', which will > fail unless the type is 'bool' already. SO the expression here cannot be a > nullptr or struct type. Before I moved it and wrote the new tests (NullTy > and StructTy), it actually did hit the assert you mentioned. Ah, perfect! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141954/new/ https://reviews.llvm.org/D141954 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
