[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

[Emilia Dreamer via Phabricator via cfe-commits]

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