[PATCH] D104680: [clang] Eliminate relational function pointer comparisons in all C++ modes
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 24 12:08:19 PDT 2021
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
More information about the cfe-commits
mailing list