[libcxx-commits] [PATCH] D58332: Erase-Like Algorithms Should Return size_type (P0646R1)

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists added inline comments.


================
Comment at: include/forward_list:1535
     iterator __e = end();
+    auto __start_count = _VSTD::distance(begin(), end());
     for (iterator __i = before_begin(); __i.__get_begin()->__next_ != nullptr;)
----------------
zoecarver wrote:
> mclow.lists wrote:
> > This is an expensive operation, it requires traversing the list.
> > And you're doing it twice - once at the beginning, and once at the end.
> I have updated them all to use unsigned integers. Should be much faster. 
You're missing the point.  It's not the signed/unsignedness that is expensive; it's the traversal. This algorithm goes from the start of the list to the end, looking for things to delete. On a 100,000 element list, (say) that takes time.  But it has to do that, because otherwise how will it know which items to delete?

But with your change, it would have to traverse that list **three times**. That's wrong.

Instead you need to "get inside" the loop, and count the items that are being erased as they are being erased.

This is specific to `forward_list`, because all the other containers have constant-time implementations of `size()`.


================
Comment at: include/list:2166
 {
+    auto __start_count = _VSTD::distance(begin(), end());
     for (iterator __i = begin(), __e = end(); __i != __e;)
----------------
`size()` is constant time.
`distance(begin(), end())` is linear.


================
Comment at: include/list:2198
 {
+    auto __start_count = _VSTD::distance(begin(), end());
     for (iterator __i = begin(), __e = end(); __i != __e;)
----------------
Same here. Use `size()`.


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

https://reviews.llvm.org/D58332