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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Sorry, this slipped through my inbox somehow.

> 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.

Here's the patch <https://reviews.llvm.org/D58332> I made to implement the first paper [1]. @mclow.lists gave me some great review comments, which apply to this patch as well. Essentially, we never want to traverse the whole range twice (i.e. `remove_if` and `distance`). It looks like that's not a problem anymore, though.

That being said, you make a good point. In all those cases, `x.distance` isn't a traversal of the container. Make sure that you're using the method, not the `std::distance` function, though.

[1]: https://reviews.llvm.org/D58332



================
Comment at: libcxx/include/functional:3084
+inline typename _Container::size_type
+__libcpp_erase_if_container(_Container& __c, _Predicate __pred) {
+  typename _Container::size_type __old_size = __c.size();
----------------
This seems like a weird place to have this function. Is there another header that would work better? 

I know you didn't make this change but, I figured I'd ask anyway.


================
Comment at: libcxx/include/functional:3085
+__libcpp_erase_if_container(_Container& __c, _Predicate __pred) {
+  typename _Container::size_type __old_size = __c.size();
+
----------------
Instead of calling size, maybe make this a count that is incremented throughout the loop? I don't think it matters much but, it might make it a bit more general. 


================
Comment at: libcxx/include/string:4397
+  auto __old_size = __str.size();
+  __str.erase(_VSTD::remove_if(__str.begin(), __str.end(), __pred),
+              __str.end());
----------------
Why not use `__libcpp_erase_if_container`?


================
Comment at: libcxx/include/vector:3405
+  auto __old_size = __c.size();
+  __c.erase(_VSTD::remove_if(__c.begin(), __c.end(), __pred), __c.end());
+  return __old_size - __c.size();
----------------
Also could use `__libcpp_erase_if_container`, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75905