rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Only minor comments, let me know if you'd like me to take another look after
addressing these, or just go ahead and commit. Thanks!
================
Comment at: clang/lib/AST/ExprCXX.cpp:1676-1677
+ ConceptDecl *NamedConcept, const ASTTemplateArgumentListInfo
*ArgsAsWritten,
+ ArrayRef<TemplateArgument> ConvertedArgs, bool IsSatisfied,
+ bool IsValueDependent)
+ : Expr(ConceptSpecializationExprClass, C.BoolTy, VK_RValue, OK_Ordinary,
----------------
Consider passing `IsSatisfied` and `IsValueDependent` as a single
`Optional<bool>` (with `None` meaning value-dependent).
================
Comment at: clang/lib/AST/ExprCXX.cpp:1719-1720
+ ->containsUnexpandedParameterPacks());
+ setInstantiationDependent(IsInstantiationDependent);
+ setContainsUnexpandedParameterPack(ContainsUnexpandedParameterPack);
+}
----------------
Please add
```
assert((!isValueDependent() || isInstantiationDependent()) && "should not be
value-dependent");
```
or similar. If we marked the expression as being value-dependent but it's not
instantiation-dependent, things would go badly wrong later (we wouldn't
substitute into it during instantiation, for example).
================
Comment at: clang/lib/Sema/SemaConcept.cpp:39
+
+ QualType Type = ConstraintExpression->getType().getNonReferenceType()
+ .getUnqualifiedType();
----------------
No need for `getNonReferenceType()` here; expressions never have reference type.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:40-41
+ QualType Type = ConstraintExpression->getType().getNonReferenceType()
+ .getUnqualifiedType();
+ if (!Context.hasSameType(Type, Context.BoolTy)) {
+ Diag(ConstraintExpression->getExprLoc(),
----------------
I think it'd be a bit better to remove the `getUnqualifiedType()` call and use
`hasSameUnqualifiedType` instead. That way the diagnostic below will preserve
the cv-qualifications (along with any type sugar that you would need to remove
to remove the qualifiers).
================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:737
+void ASTStmtReader::VisitConceptSpecializationExpr(
+ ConceptSpecializationExpr *E) {
----------------
Please can you add some basic test coverage for the serialization code?
================
Comment at: clang/test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:1
-// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -verify %s
+// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -verify -triple x86_64-linux-gnu
%s
----------------
Can we remove `-fconcepts-ts` from the `RUN` line?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41217/new/
https://reviews.llvm.org/D41217
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
