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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius marked 4 inline comments as done.
curdeius added a comment.

Hopefully I answered your questions, @zoecarver.



================
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();
----------------
zoecarver wrote:
> 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.
Well, this function is used in `map`, `set`, `unordered_map` and `unordered_set` headers. The common includes of these are: `__config`, `__node_handle`, `functional` and `version`. IMO, `__config` and `version` are really not the place for this. `functional` seems okayish. I'm unsure about `__node_handle`.
I'd rather leave it as is.

I'd rather be reluctant as to creating another `__header` for this particular function, but I don't know what is the policy of using additional headers in libc++.


================
Comment at: libcxx/include/functional:3085
+__libcpp_erase_if_container(_Container& __c, _Predicate __pred) {
+  typename _Container::size_type __old_size = __c.size();
+
----------------
zoecarver wrote:
> 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. 
I have no opinion on this. Both seem ok.


================
Comment at: libcxx/include/string:4397
+  auto __old_size = __str.size();
+  __str.erase(_VSTD::remove_if(__str.begin(), __str.end(), __pred),
+              __str.end());
----------------
zoecarver wrote:
> Why not use `__libcpp_erase_if_container`?
I think that `__libcpp_erase_if_container` would be inefficient for vector-like containers, as you would call erase n times (n = count of removed elements), hence moving left all elements after the erased one n times.


================
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();
----------------
zoecarver wrote:
> Also could use `__libcpp_erase_if_container`, no?
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75905