[libcxx-commits] [PATCH] D75905: [libc++][P1115][C++20] Improving the Return Value of Erase-Like Algorithms II: Free erase/erase if.

Thu Mar 12 03:46:56 PDT 2020

curdeius added a comment.

In D75905#1918677 <https://reviews.llvm.org/D75905#1918677>, @zoecarver wrote:

> It would be nice if we could remove the calls to `std::difference` and replace them with a count variable so that we only have to iterate once. It may require adding a `__libcpp_ erase` function, though.

Where do you see that the iteration takes place twice? `std::distance` on LegacyRandomAccessIterators is an O(1) operation. And all of basic_string, deque and vector iterators belong to this category.
What do you mean by adding a count variable? `remove` and `remove_if` functions do not provide us with a count.

In D75905#1918626 <https://reviews.llvm.org/D75905#1918626>, @EricWF wrote:

> I think there is a bug in the paper, but I think the return type should be `difference_type` not `size_type`.

I'm not sure if it's a bug. I think that a coherence with other APIs was intended. Apart the additions in P0646 <http://wg21.link/P0646>, there is already pre-C++11 `size_type map::erase(key_type)` as well as `size_type set::erase(key_type)`.

Otherwise, to be coherent with `__libcpp_erase_if_container` and avoid casting `difference_type` to `size_type`, I'd suggest returning `old_size - size()` instead of using `distance`.

================
Comment at: libcxx/include/functional:3087
+
+  for (typename _Container::iterator __iter = __c.begin(), __last = __c.end();
+       __iter != __last;) {
----------------
EricWF wrote:
> We need to avoid calling an overloaded comma operator here.
Done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75905