[PATCH] D104680: [clang] Eliminate relational function pointer comparisons in all C++ modes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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