[libcxx-commits] [PATCH] D110515: [libc++] [compare] Named comparison functions, is_eq etc.

[Louis Dionne via Phabricator via libcxx-commits]

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

LGTM but please consider my `inline` suggestion!

Thanks a lot for the patch, the `operator<=>` support has been in need of some attention for a while.



================
Comment at: libcxx/include/__compare/is_eq.h:23-28
+constexpr bool is_eq(partial_ordering __c) noexcept { return __c == 0; }
+constexpr bool is_neq(partial_ordering __c) noexcept { return __c != 0; }
+constexpr bool is_lt(partial_ordering __c) noexcept { return __c < 0; }
+constexpr bool is_lteq(partial_ordering __c) noexcept { return __c <= 0; }
+constexpr bool is_gt(partial_ordering __c) noexcept { return __c > 0; }
+constexpr bool is_gteq(partial_ordering __c) noexcept { return __c >= 0; }
----------------
Quuxplusone wrote:
> (1) Not marked `_LIBCPP_HIDE_FROM_ABI`, because I can't imagine their ABI will ever change, but I don't really understand the meaning of the marking, and maybe it's better to mark them; @ldionne opinion?
> 
> (2) TIL (again) that although `constexpr` on a global variable implies `static`, `constexpr` on a function implies `inline`. So it's correct that these header-defined functions aren't explicitly marked `inline`; they still have the right behavior.
Yes, we want `_LIBCPP_HIDE_FROM_ABI` here, like we do everywhere else. I agree in this case we could probably skip it since they are probably never going to change, but we might as well be consistent.

I wasn't aware of the `constexpr` implies `inline` bit. I find that surprising and weird. If it were me, I wouldn't rely on that corner case and I'd mark them as `inline` explicitly, since I expect most readers of the code will go "wut?" if we don't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110515