rymiel marked 2 inline comments as done. rymiel added inline comments.
================ Comment at: clang/lib/Sema/SemaConcept.cpp:116 // primary-expression, and complain that it is of non-bool type. - (NextToken.is(tok::l_paren) && + (NextToken.is(tok::l_paren) && !IsLambdaRequiresClause && (IsTrailingRequiresClause || ---------------- erichkeane wrote: > rymiel wrote: > > erichkeane wrote: > > > I'd like to expand on the comment above in this case. Also, since we > > > don't decide that this is a trailing requires clause in the lambda > > > parsing, we should probably make this more specific in this condition. I > > > THINK we still want to do the bin-op precedence condition in this case, > > > right? > > > I'd like to expand on the comment above in this case. > > > > Yes, that's a very good call, doing that now. > > > > > Also, since we don't decide that this is a trailing requires clause in > > > the lambda parsing, we should probably make this more specific in this > > > condition. > > > > I'm not 100% sure what you mean here... > > > > > I THINK we still want to do the bin-op precedence condition in this case, > > > right? > > > > I think it's still being done, but it's not very clear from the mess of a > > logic expression > So my concern is that this is a 'top level' condition here, rather than being > 'more specific', but you're right, this is a little confusing logic, and I'm > afraid I perhaps got confused as well. So here is the logic as it sits in > your patch. > ``` > ( > NextToken.is(tok::l_paren) > && > !IsLambdaRequiresClause > && > ( > IsTrailingRequiresClause > || > ( > Type->isDependentType() //#1 > && > isa<UnresolvedLookupExpr>(ConstraintExpression) > ) > || > Type->isFunctionType() > || > Type->isSpecificBuiltinType(BuiltinType::Overload) > ) > ) > || > getBinOpPrecedence(NextToken.getKind(), /*GreaterThanIsOperator=*/true, > getLangOpts().CPlusPlus11) > prec::LogicalAnd; > ``` > > I suspect we don't want to have this skip the `getBinOpPrecedence`, which I > think you're right, works correctly. I'm a bit concerned about your patch > skippinjg the `isFunctionType` and `isSpecificBuiltinType` branches. > > The one in your reproducer hits only the `isDependentType() && > isa<ULE>(ConstraintExpr)`, correct? So unless you have a more specific > example where it should also apply when the type is a function/overload set, > I suspect the `!IsLambdaRequiresClause` would be best placed in with the ULE > check (~#1). > > Does that make sense? Yes, that makes a lot of sense, thank you for pointing that out! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146140/new/ https://reviews.llvm.org/D146140 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits