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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general I'm happy, one nit remaining. I like to see the CI green before approving.



================
Comment at: libcxx/include/__memory/shared_ptr.h:1240
+strong_ordering
+operator<=>(shared_ptr<_Tp> const& __a, shared_ptr<_Up> const& __b) _NOEXCEPT
+{
----------------
Mordante wrote:
> Mordante wrote:
> > The macro only needs to be used in code that works in C++03.
> please use `__x` and `__y` for consistency.
Seems you missed the `noexcept` here, but I see it's fixed at the other location.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp:47
 
 int main(int, char**)
 {
----------------
avogelsgesang wrote:
> Mordante wrote:
> > Partly pre-existing, but I miss tests to validate the return type and that the function is `noexcept`. Can you add tests for this? See D128603 for examples.
> > 
> > 
> Added calls to `AssertComparisonsAreNoexcept`, `AssertComparisonsReturnBool`, `AssertOrderAreNoexcept`, etc.
> 
> My code required additional `#ifdef`s, though.
> Would it make sense to introduce `AssertOrderReturn` also for dialects < C++20, and let it gracefully degrade to `AssertComparisonsReturnBool` on those earlier C++ versions?
No I think that doesn't make sense since it changes the macro depending on the language version used. In libc++ and its tests we're used to using `#if` often. Sometimes we have some helper macros, but only for generic things like `CONSTEXPER_AFTER_XX`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130852