[PATCH] D138914: Make evaluation of nested requirement consistent with requires expr.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:428
+      : Requirement(RK_Nested,
+                    Constraint && Constraint->isInstantiationDependent(),
+                    Constraint && Constraint->containsUnexpandedParameterPack(),
----------------
erichkeane wrote:
> I'm a little concerned that we're changing to a situation where the constraint in a nested-requirement can be null like this.  The other cases it can be null we consider it a 'substitution failure' (see the 1st constructor).
> 
> It seems to me that we perhaps need a different state here for that.
I still have this concern, we need to make sure that NestedRequirement is written to consider this type of failure, it likely needs a separate constructor that contains information about the failure.  We probably want at least some sort of stored diagnostic here, sot hat we dont lose the output we have below.


================
Comment at: clang/test/CXX/expr/expr.prim/expr.prim.req/nested-requirement.cpp:23
+    template<typename U> requires requires (U u) { requires sizeof(u) == sizeof(T); } 
+    // expected-note at -1 {{because substituted constraint expression is ill-formed: invalid application of 'sizeof' to an incomplete type 'void'}}
     struct r4 {};
----------------
Losing the extra context here is unfortunate as well... why do we lose this info?


================
Comment at: clang/test/CXX/expr/expr.prim/expr.prim.req/nested-requirement.cpp:94
+template<class T> void fooAmps1() requires Amps1<T> {}
+// expected-note at -1 {{candidate template ignored: constraints not satisfied [with T = int]}} \
+// expected-note at -1 {{because 'int' does not satisfy 'Amps1'}}
----------------
Can you re-arrange this test with the #NAME type bookmarks so that the instantiation notes/etc are all arranged with the error they are associated with?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138914