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

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added inline comments.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:428
+      : Requirement(RK_Nested,
+                    Constraint && Constraint->isInstantiationDependent(),
+                    Constraint && Constraint->containsUnexpandedParameterPack(),
----------------
erichkeane wrote:
> 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.
NestedRequirement will no more capture a substitution failure. I am planning to remove all references to SubstitutionDiagnostics for it. I have **temporarily** added unreachable tags to the references of SubstitutionDiagnostics. 

If this looks fine to you then I would move forward with refactoring it to remove all support for SubstitutionDiagnostics in NestedRequirement. WDYT ?


================
Comment at: clang/lib/Sema/SemaConcept.cpp:172
       //    operand is satisfied.
-      return BO.recreateBinOp(S, LHSRes);
+      // LHS is instantiated while RHS is not. Skip creating invalid BinaryOp.
+      return LHSRes;
----------------
For context: This is not necessarily invalid but more importantly the BinaryOp cannot be constant evaluated as it would have dependent RHS.


================
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 {};
----------------
erichkeane wrote:
> Losing the extra context here is unfortunate as well... why do we lose this info?
This is now restored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138914