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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann 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());
----------------
`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?


================
Comment at: clang/lib/Sema/SemaConcept.cpp:383
         if (!S.CheckConstraintExpression(SubstitutedExpression.get()))
-          return ExprError();
+          return ExprError(); // convert to bool here?
 
----------------
This comment appears to be unnecessary.


================
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
+
----------------
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).


================
Comment at: clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp:28
+  f2(0); // #F2INST
+}  
----------------
These tests look good.

I think we should also exercise nested requirements to ensure that behavior is consistent:
  template<typename T> requires requires {
    requires S<T>{};
  }
  void f3(T);
  void f3(int);

Perhaps also add some examples where `&&` does not denote a conjunction of atomic constraints. For example:
  template<typename T> requires (bool(1 && 2))
  void f4(T);
  void f4(int);


================
Comment at: libcxx/include/module.modulemap.in:750-753
+      module common_reference_with      { 
+        private header "__concepts/common_reference_with.h" 
+        export type_traits.common_reference
+      }
----------------
I agree with this being done in a separate commit.


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

https://reviews.llvm.org/D141954