[PATCH] D41217: [Concepts] Concept Specialization Expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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