[libcxx-commits] [PATCH] D131372: [libc++][spaceship] Implement std::variant::operator<=>

Mark de Wever via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Aug 10 09:21:22 PDT 2022


Mordante added a comment.

This is getting close to be ready, some minor issue remain.



================
Comment at: libcxx/include/__variant/monostate.h:33
+
+inline _LIBCPP_INLINE_VISIBILITY
+constexpr strong_ordering operator<=>(monostate, monostate) noexcept { return strong_ordering::equal; }
----------------
mumbleskates wrote:
> avogelsgesang wrote:
> > as I learned today in a different review, `_LIBCPP_INLINE_VISIBILITY` was superseded by `_LIBCPP_HIDE_FROM_ABI` (see https://libcxx.llvm.org/DesignDocs/VisibilityMacros.html#visibility-macros)
> i've replaced all cases of `_LIBCPP_INLINE_VISIBILITY` with `_LIBCPP_HIDE_FROM_ABI` and removed `inline` in every case where the function itself was either `constexpr` or a template function.
> 
> this establishes consistency and modern practice at the expense of ~90 lines of noise. let me know if we ought to fork that to a new differential.
Thanks! FYI I normally prefer those cleanups in a separate commit, but since they are simple changes I'm fine keeping them this time.


================
Comment at: libcxx/include/__variant/monostate.h:33
+
+inline _LIBCPP_INLINE_VISIBILITY
+constexpr strong_ordering operator<=>(monostate, monostate) noexcept { return strong_ordering::equal; }
----------------
Mordante wrote:
> mumbleskates wrote:
> > avogelsgesang wrote:
> > > as I learned today in a different review, `_LIBCPP_INLINE_VISIBILITY` was superseded by `_LIBCPP_HIDE_FROM_ABI` (see https://libcxx.llvm.org/DesignDocs/VisibilityMacros.html#visibility-macros)
> > i've replaced all cases of `_LIBCPP_INLINE_VISIBILITY` with `_LIBCPP_HIDE_FROM_ABI` and removed `inline` in every case where the function itself was either `constexpr` or a template function.
> > 
> > this establishes consistency and modern practice at the expense of ~90 lines of noise. let me know if we ought to fork that to a new differential.
> Thanks! FYI I normally prefer those cleanups in a separate commit, but since they are simple changes I'm fine keeping them this time.
> as I learned today in a different review, `_LIBCPP_INLINE_VISIBILITY` was superseded by `_LIBCPP_HIDE_FROM_ABI`

Historically we haven't always resolved our tech-debt, but we're better at it nowadays :-)




================
Comment at: libcxx/include/__variant/monostate.h:34
+inline _LIBCPP_INLINE_VISIBILITY
+constexpr strong_ordering operator<=>(monostate, monostate) noexcept { return strong_ordering::equal; }
+
----------------
avogelsgesang wrote:
> avogelsgesang wrote:
> > I usually try to keep the order between the synopsis and the implementation in sync.
> > In this case, the synopsis has `operator<=>` after the removed comparisons. I think we should also use that order here in the implementation, i.e. switch the `#if` branches
> Let's continue this discussion in https://reviews.llvm.org/D131363#inline-1263505
This isn't something we really do in libc++, I don't object against it but I don't see it as mandatory.
(I agree there's a benefit to do it.)


================
Comment at: libcxx/include/variant:747
   public:                                                                      \
-    inline _LIBCPP_INLINE_VISIBILITY                                           \
+    _LIBCPP_HIDE_FROM_ABI                                           \
     explicit constexpr __union(__valueless_t) noexcept : __dummy{} {}          \
----------------
please fix the formatting, can you also fix line 742?


================
Comment at: libcxx/include/variant:1651
+  using __result_t = common_comparison_category_t<compare_three_way_result_t<_Types>...>;
+  if (__lhs.valueless_by_exception() && __rhs.valueless_by_exception()) return strong_ordering::equal;
+  if (__lhs.valueless_by_exception()) return strong_ordering::less;
----------------
The `return` is normally on its own line.

FYI `git clang-format HEAD^` can be used to format the changes in the last commit and then you can amend the wanted changes. This is basically what I do to format only relevant changes.


================
Comment at: libcxx/include/variant:184
   constexpr bool operator==(monostate, monostate) noexcept;
-  constexpr bool operator!=(monostate, monostate) noexcept;
+  constexpr bool operator!=(monostate, monostate) noexcept;             // until C++20
+  constexpr bool operator<(monostate, monostate) noexcept;              // until C++20
----------------
mumbleskates wrote:
> Mordante wrote:
> > we typically use `remove in` instead of `until` (like cppreference).
> i see 35 instances of "// until c++##" and 41 instances of "// removed in c++##" in v1/include. cppreference uses the two interchangeably, typically putting "until" next to the full signature (as we are writing here) and "removed in" next to the short name. (see: [pair/operator_cmp](https://en.cppreference.com/w/cpp/utility/pair/operator_cmp), [variant/operator_cmp](https://en.cppreference.com/w/cpp/utility/tuple/operator_cmp), [monostate](https://en.cppreference.com/w/cpp/utility/variant/monostate)).
I see, the code I looked at thus-far uses `removed in`. But feel free to keep this as is.


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp:18
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
Can you move this to line 8?


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp:73
+    V v1;
+    makeEmpty(v1);
+    assert(testOrder(v1, v2, std::weak_ordering::equivalent));
----------------
Why switch the order of V1 and V2?


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp:106
+    assert(testOrder(v1, v2, Order(std::strong_ordering::greater)));
+  }
+
----------------
Can you add a test for `std::partial_ordering::unordered`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131372



More information about the libcxx-commits mailing list