This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement `operator<=>` for `error_category`
ClosedPublic

Authored by avogelsgesang on Aug 7 2022, 10:15 AM.

Details

Summary

Implements part of P1614R2 "The Mothership has Landed"

Diff Detail

Event Timeline

avogelsgesang created this revision.Aug 7 2022, 10:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 10:15 AM
avogelsgesang requested review of this revision.Aug 7 2022, 10:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 10:15 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Update review link

mumbleskates added inline comments.Aug 7 2022, 8:10 PM
libcxx/include/system_error
37

nit, per most-common pattern and personal preference

227

nit: Most checks of _LIBCPP_STD_VER are in terms of >, such that these blocks would be reversed, with C++20-onwards behavior gated behind _LIBCPP_STD_VER > 17. For consistency's sake I think it would be good to match that.

avogelsgesang added inline comments.
libcxx/include/system_error
227

I usually prefer it this way around, because I value "keeping the order between synopsis and implementation consistent" over "consistently use _LIBCPP_STD_VER > 17".

Do you think swapping the #if here is worth it, although that would mean that the order in the synopsis no longer matches the order in the implementation?

(related: https://reviews.llvm.org/D131372#inline-1263658)

mumbleskates added inline comments.Aug 8 2022, 1:42 PM
libcxx/include/system_error
227

okay, i can see that rationale. in my previous differentials (<=> for pair and tuple) i implemented it in the order you've seen me do, preferring a > 17 check and putting modern operators (==, <=>) first and outdated ones in the #else branch in cases where <=> obsoletes the other operators.

understanding your rationale i have a weaker preference now, still leaning towards whatever our conventional momentum is.

avogelsgesang added inline comments.
libcxx/include/system_error
227

@philnik @Mordante @ldionne @var-const does anyone of you have a preference here? Not sure which (purely stylistic) tradeoff we want to take here...

Mordante accepted this revision.Aug 14 2022, 10:00 AM

LGTM modulo some nits.

libcxx/include/system_error
227

I have a preference, but we don't a required style. Traditionally the > xx was more common.

237
238

This requires including __memory/addressof.h. This guards against overloading operator&, not a real issue here but in other places, like containers it is.

This revision is now accepted and ready to land.Aug 14 2022, 10:00 AM
avogelsgesang marked 5 inline comments as done.

address comments. Will merge as soon as CI finished

libcxx/include/system_error
227

I have a preference

You didn't mention what preference that is? I assume it is > xx, i.e. in line with the "traditional" style?

I updated the review with to use >xx now

Mordante added inline comments.Aug 16 2022, 9:49 AM
libcxx/include/system_error
227

I didn't mention it since code reviews shouldn't be about getting the code in the exact preferred style of the reviewer. Since we already use different styles there's no "project style".

But you guessed right and I personally prefer > XX, but a portion of that is just because I'm used to it.