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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
Comment at: libcxx/include/system_error:227
 
+#if _LIBCPP_STD_VER <= 17
+
----------------
Mordante wrote:
> 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.
> 
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131363