[libcxx-commits] [PATCH] D129657: [libc++][ranges] implement `std::ranges::set_union`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.
This revision is now accepted and ready to land.

LGTM with very minor nits. Thanks!



================
Comment at: libcxx/include/__algorithm/set_union.h:63
 template <class _InputIterator1, class _InputIterator2, class _OutputIterator, class _Compare>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-_OutputIterator
-set_union(_InputIterator1 __first1, _InputIterator1 __last1,
-          _InputIterator2 __first2, _InputIterator2 __last2, _OutputIterator __result, _Compare __comp)
-{
-    typedef typename __comp_ref_type<_Compare>::type _Comp_ref;
-    return _VSTD::__set_union<_Comp_ref>(__first1, __last1, __first2, __last2, __result, __comp);
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 _OutputIterator set_union(
+    _InputIterator1 __first1,
----------------
If you're already refactoring this line, might as well use `_LIBCPP_HIDE_FROM_ABI` instead of `_LIBCPP_INLINE_VISIBILITY`.


================
Comment at: libcxx/include/__algorithm/set_union.h:71
+  typedef typename __comp_ref_type<_Compare>::type _Comp_ref;
+  return std::__set_union<_Comp_ref>(__first1, __last1, __first2, __last2, __result, __comp).__out_;
 }
----------------
Nit: move?


================
Comment at: libcxx/include/__algorithm/set_union.h:82
+  return std::set_union(
+      __first1,
+      __last1,
----------------
Nit: move?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.union/ranges_set_union.pass.cpp:46
+      std::ranges::set_union(
+          std::forward<InIter1>(inIter1),
+          std::forward<Sent1>(sent1),
----------------
Nit: include `<utility>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129657