[libcxx-commits] [PATCH] D129806: [libc++] Implement ranges::replace_copy{, _if}

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I just looked at it mainly out of curiosity, but I didn't do a in-depth review. But some comments.



================
Comment at: libcxx/.clang-format:71
 
-PenaltyIndentedWhitespace: 61
+PenaltyIndentedWhitespace: 2
 
----------------
Please remove this, it should be in a separate patch. Which I did in D129441 on your request.


================
Comment at: libcxx/include/__algorithm/ranges_replace_copy.h:40
 
-struct __fn {
-
-  template <input_iterator _InIter, sentinel_for<_InIter> _Sent, class _Type1, class _Type2,
-            output_iterator<const _Type2&> _OutIter, class _Proj = identity>
-  requires indirectly_copyable<_InIter, _OutIter> &&
-           indirect_binary_predicate<ranges::equal_to, projected<_InIter, _Proj>, const _Type1*>
-  _LIBCPP_HIDE_FROM_ABI constexpr
-  replace_copy_result<_InIter, _OutIter>
-  operator()(_InIter __first, _Sent __last, _OutIter __result, const _Type1& __old_value, const _Type2& __new_value,
+  struct __fn {
+    template <input_iterator _InIter,
----------------
huixie90 wrote:
> puzzle: why does clang-format add two spaces here. I was told don't have spaces here
This is the inner namespace indention clang-format setting. We're considering to remove that.


================
Comment at: libcxx/test/libcxx/algorithms/ranges_robust_against_copying_projections.pass.cpp:221
+    (void)std::ranges::upper_bound(first, last, value, Less(), Proj(&copies)); assert(copies == 0);
+    (void)std::ranges::upper_bound(a, value, Less(), Proj(&copies)); assert(copies == 0);
 
----------------
It would be nice to commit the upper and lower bound and search separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129806