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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
