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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D104680#2839309 <https://reviews.llvm.org/D104680#2839309>, @rsmith wrote:

> I would prefer to split this into two changes:
> ...
> Does that sound reasonable?

Yeah that is fine, totally understand ;)



================
Comment at: clang/lib/Sema/SemaExpr.cpp:11815
+    if (IsError)
+      return Opc != BO_Cmp ? Context.getLogicalOperationType() : QualType();
+  }
----------------
rsmith wrote:
> 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.)
Hmm. My thinking here is that it would be totally appropriate to error recovery with a 'strong_ordering' type, since that can decay into all the other orderings, making it kind of a safe bet that it would not cause further problems.

The idea of a separate type for error recovery is intriguing. But I wonder if it would be appropriate in some cases, but not all of them.
* The cases where all the options are equally bad and we have no good guesses would fit the bill, but this is not the case here.
* When we have a good safe bet, we can in principle just use that type. But maybe it would make sense to add a bit to it that means something like "but don't take this too seriously", which could be used to selectively suppress some diagnostics downstream.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104680/new/

https://reviews.llvm.org/D104680