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

[Erich Keane via Phabricator via cfe-commits]

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