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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
