[PATCH] D103850: [clang] diagnose instead of assert for defaulted comparison return type deduction.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/test/CXX/class/class.compare/class.spaceship/p2.cpp:200
+  struct b3 {
+    auto operator<=>(b3 const &) const = default; // expected-error {{cannot be deduced because three-way comparison for member 'f' would compare as builtin type 'void (*)()' which is not currently supported}}
+    // expected-warning at -1 {{implicitly deleted}}
----------------
rsmith wrote:
> I think three-way comparison for function pointers is ill-formed, and that the problem in this case is that [over.built] lists candidates for which a three-way comparison is invalid. So our overload resolution succeeds, but results in a meaningless builtin candidate with an undefined return type.
> 
> I think the behavior here after this patch is fine -- we should treat the comparison as deleted -- but the diagnostic is wrong. This isn't a "not currently supported" case, this is a "not a valid expression" case, per [class.compare.default]/3.2 (except that the wording currently gets this wrong and doesn't actually say what happens here, because we're actually in [class.compare.default]/3.1). Though perhaps the better fix would be to not even include a builtin candidate for `operator<=>(void (*)(), void (*)())`?
Yeah this was part of my plan to give the correct diagnostic for function pointers in the next patch in the series:
https://reviews.llvm.org/D103855

The idea on this one is to produce an error, is the TU will not compile, for built-in types not supported.

After the next one, we just implicitly delete the comparison for function pointers, and leave the error for other unimplemented built-in types.

I think this makes more sense, even if we manage to implement this deduction for all built-in types currently available, it would still be too easy for another patch to introduce a new built-in types and forget to support it here.

And it does make sense to error on these cases than to simply pick an arbitrary answer and churn along, I think 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103850