rsmith added a comment.
I would prefer to split this into two changes:
1. Stop providing a builtin overload candidate `R operator<=>(P, P)` where `P`
is a function pointer type, to be consistent with the behavior of the built-in
`<=>` operator.
2. Stop providing `<`/`<=`/`>`/`>=` support for function pointer types.
I think (1) is a (relatively) safe change that we can make immediately with
very little roll-out concern. `<=>` is new, and the built-in candidate would
never work anyway, so this is unlikely to cause problems for any real code, and
has had clear and unanimous support in C++ committee discussions. So let's do
that.
I think (2) is a lot riskier for existing code, and I think there's at least
some chance that the C++ committee will end up effectively asking for change
(1) but not change (2). It also looks like this part of the change will require
a matching change in the standard library, because `std::less` on function
pointer types would presumably still be required to work. So I think a safer
approach here would be to add an enabled-by-default warning for relational
comparisons of function pointers now, and return to this later to make the
comparisons ill-formed if it still looks like the C++ committee is moving in
that direction.
Does that sound reasonable?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:11815
+ if (IsError)
+ return Opc != BO_Cmp ? Context.getLogicalOperationType() : QualType();
+ }
----------------
mizvekov wrote:
> Quuxplusone wrote:
> > Peanut gallery says: Is `QualType()` the right "placeholder" to return
> > here? IIUC, this is the situation where we've diagnosed an ill-formed
> > expression and are just trying to do error-recovery: if the expression
> > looks like `x < y` then we assume the programmer wants it to return `bool`,
> > and if the expression looks like `x <=> y` then we assume the programmer
> > wants it to return... `QualType()`? Is that the same thing we'd do for e.g.
> > `x + y` or `undeclaredfunction(x)`? (If so, good, I think.)
> So right now, this is the same we are doing for three-way comparison between
> builtin types we do not support / recognize. Check just below in this same
> function. Just look for the call to `getComparisonCategoryForBuiltinCmp`.
>
> I agree completely with you and I was going to raise the same point, but I
> would prefer we changed both places at once instead of fixing it just here,
> so I think this should be a follow up task: Find something more appropriate
> to return here.
A null `QualType` is in this case treated as an indication that the comparison
is invalid, recovery failed, and the callee has already produced a diagnostic.
(It's a bit sad that we don't generally use a separate `TypeResult` for this
kind of thing in order to distinguish between the "no type" answer and the
"invalid and I've produced a diagnostic" answer, but if we wanted to change
that I think we should aim to do it holistically across all of Clang.)
================
Comment at: clang/lib/Sema/SemaExpr.cpp:11807
+ RHSType->isFunctionPointerType()) {
+ // Valid unless a relational comparison of function pointers
+ bool IsError = getLangOpts().CPlusPlus;
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:9226
OpBuilder.addEqualEqualOrNotEqualMemberPointerOrNullptrOverloads();
- LLVM_FALLTHROUGH;
+ OpBuilder.addGenericBinaryPointerOrEnumeralOverloads(false);
+ OpBuilder.addGenericBinaryArithmeticOverloads();
----------------
An `/*IsOrdered*/` comment here and below (or use of an enum) would aid
readability.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104680/new/
https://reviews.llvm.org/D104680
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
