[PATCH] D41217: [Concepts] Concept Specialization Expressions

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2404
+def err_concept_non_constant_constraint_expression : Error<
+  "concept specialization '%0' resulted in a non-constant expression '%1'.">;
+def err_non_bool_atomic_constraint : Error<
----------------
Extra period `.` here


================
Comment at: lib/Serialization/ASTReaderStmt.cpp:4053
+    case EXPR_CONCEPT_SPECIALIZATION:
+      S = new (Context) ConceptSpecializationExpr(Context);
+      break;
----------------
Peanut gallery says: All the other cases look like
    S = new (Context) FooExpr(Context, Empty);
or
    S = new (Context) FooExpr::CreateEmpty(Context);
not
    S = new (Context) FooExpr(Context);
Is the visual difference in this case significant?


================
Comment at: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:12
+
+template<typename T> concept C3 = sizeof(*T{}) == 4;
+static_assert(C3<int*>);
----------------
"test/Parser/cxx-concept-declaration.cpp" has some syntax tests for "value-concepts" of the form `template<bool x> concept...`. Would it make sense to add some semantic tests for `template<bool x> concept...` in or near this test file?
(This could plausibly be "out of scope." I merely mention it.)


================
Comment at: test/Parser/cxx-concept-declaration.cpp:36
+template<typename T> concept C11 = sizeof(T); // expected-error {{atomic constraint 'sizeof(T)' must be of type 'bool' (found 'unsigned long')}}
+template<typename T> concept C12 = T{};
+template<typename T> concept C13 = (bool&&)true;
----------------
Peanut gallery says: IIUC, C12<T> is satisfied whenever T has a constexpr constructor and a constexpr implicit conversion to `bool` that returns `true`?  Or (because this is an atomic constraint and atomic constraints don't do conversions, as we can see on line 31 above) is C12<T> *actually* never satisfiable (but there's no diagnostic expected here because `T{}` is dependent)?


Repository:
  rC Clang

https://reviews.llvm.org/D41217