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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
