[PATCH] D141954: Forbid implicit conversion of constraint expression to bool

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
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());
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141954/new/

https://reviews.llvm.org/D141954