[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:
> `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.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:383
         if (!S.CheckConstraintExpression(SubstitutedExpression.get()))
-          return ExprError();
+          return ExprError(); // convert to bool here?
 
----------------
tahonermann wrote:
> This comment appears to be unnecessary.
Woops!  I thought that wasn't mine :D  


================
Comment at: clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp:3-4
+
+template<typename T> concept C =
+sizeof(T) == 4 && !true;      // requires atomic constraints sizeof(T) == 4 and !true
+
----------------
tahonermann wrote:
> I'm not sure what this is intending to exercise. `C` isn't used elsewhere; this just appears to validate that a well-formed concept declaration is accepted (even if it is one that will always evaluate as false). I suggest modifying it to drop `== 4 && !true` and then validate that a diagnostic is issued (which might require a use of the concept).
This is exactly out of the standard, which is why I copied it.  however, a test for the diagnostic version seems like a good idea.


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

https://reviews.llvm.org/D141954