[libcxx-commits] [PATCH] D131363: [libc++] Implement `operator<=>` for `error_category`

Mark de Wever via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sun Aug 14 10:00:34 PDT 2022


Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM modulo some nits.



================
Comment at: libcxx/include/system_error:227
 
+#if _LIBCPP_STD_VER <= 17
+
----------------
avogelsgesang wrote:
> mumbleskates wrote:
> > avogelsgesang wrote:
> > > mumbleskates wrote:
> > > > 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.
> > > 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)
> > 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.
> @philnik @Mordante @ldionne @var-const does anyone of you have a preference here? Not sure which (purely stylistic) tradeoff we want to take here...
I have a preference, but we don't a required style. Traditionally the `> xx` was more common.



================
Comment at: libcxx/include/system_error:237
+
+    _LIBCPP_INLINE_VISIBILITY
+    strong_ordering operator<=>(const error_category& __rhs) const noexcept {return compare_three_way()(this, &__rhs);}
----------------



================
Comment at: libcxx/include/system_error:238
+    _LIBCPP_INLINE_VISIBILITY
+    strong_ordering operator<=>(const error_category& __rhs) const noexcept {return compare_three_way()(this, &__rhs);}
+
----------------
This requires including `__memory/addressof.h`. This guards against overloading `operator&`, not a real issue here but in other places, like containers it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131363



More information about the libcxx-commits mailing list