erichkeane 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 ||
----------------
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?
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
