Page MenuHomePhabricator

[clang] diagnose instead of assert for defaulted comparison return type deduction.
Needs ReviewPublic

Authored by mizvekov on Mon, Jun 7, 3:30 PM.

Details

Reviewers
rsmith
Summary

When deducing the return type of defaulted three-way comparison, instead
of asserting in the case of unsupported builtin types, just diagnose it.

For now, we add a test case for function pointers which will produce
slightly incorrect diagnostics. In future work, we will stop adding
function pointers to the candidate set and get the correct diagnostics.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Depends on D103760

Diff Detail

Event Timeline

mizvekov requested review of this revision.Mon, Jun 7, 3:30 PM
mizvekov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 7, 3:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov added inline comments.Mon, Jun 7, 5:04 PM
clang/lib/Sema/SemaDeclCXX.cpp
7876

I think this should be an error because otherwise we would be accepting and giving this program a different semantic just because of some not-implemented feature.
But I can see the downside that we could potentially reject a program which is not affected by the operator being implicitly deleted.

Thoughts?

rsmith added inline comments.Wed, Jun 16, 10:39 AM
clang/test/CXX/class/class.compare/class.spaceship/p2.cpp
200

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 (*)())?

mizvekov added inline comments.Wed, Jun 16, 10:53 AM
clang/test/CXX/class/class.compare/class.spaceship/p2.cpp
200

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