This is an archive of the discontinued LLVM Phabricator instance.

Erase-Like Algorithms Should Return size_type (P0646R1)
ClosedPublic

Authored by zoecarver on Feb 17 2019, 3:12 PM.

Details

Summary

This patch changes the return type of "Erase-Like Algorithms" to size_type and updates tests accordingly.

For reference P0646R1.

Diff Detail

Event Timeline

zoecarver created this revision.Feb 17 2019, 3:12 PM
mclow.lists requested changes to this revision.Apr 2 2019, 10:22 PM

You're going to have to find a different approach here.
Traversing the list to see how the size has changed is not acceptable.

include/forward_list
1536

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.

include/list
2164

Just call __deleted_notes.size() It has constant complexity.

This revision now requires changes to proceed.Apr 2 2019, 10:22 PM
zoecarver marked 2 inline comments as done.Apr 4 2019, 7:58 AM
zoecarver added inline comments.
include/forward_list
1536

I have updated them all to use unsigned integers. Should be much faster.

include/list
2164

Good call, thanks.

zoecarver updated this revision to Diff 193722.Apr 4 2019, 8:21 AM

Improve performance by using simple counters.

zoecarver marked an inline comment as done.Apr 4 2019, 8:22 AM
zoecarver added inline comments.
test/libcxx/diagnostics/nodiscard_extensions.pass.cpp
16

These showed up when I pulled from master. I am happy to get rid of them if needed.

mclow.lists added inline comments.Apr 4 2019, 8:25 AM
include/forward_list
1536

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

include/list
2172

size() is constant time.
distance(begin(), end()) is linear.

2203

Same here. Use size().

Ok, so your update and my comments passed in flight. I'll try again :-)

include/list
2172

same as L2203. Just use size()

2203

You don't have to do this work for list, because it has a constant-time size() member.

Just call size() at the start, and again at the end, and return the difference.

A meta-question:
Why does list<_Tp, _Alloc>::remove(const value_type& __x) gather all the deleted nodes into a separate list, while
list<_Tp, _Alloc>::remove_if(_Pred __pred) and list<_Tp, _Alloc>::unique() just delete them as they go?

they're all the same operation under the hood.
[ I know you didn't create this difference. ]

mclow.lists added a comment.EditedApr 4 2019, 8:58 AM

A meta-question:
Why does list<_Tp, _Alloc>::remove(const value_type& __x) gather all the deleted nodes into a separate list, while
list<_Tp, _Alloc>::remove_if(_Pred __pred) and list<_Tp, _Alloc>::unique() just delete them as they go?

they're all the same operation under the hood.
[ I know you didn't create this difference. ]

And the reason is:
The value_type &x passed into remove might refer to an item in the list itself, and if we destroy the elements as we go, we will destroy the value that we're comparing against. This is not the case for unique, but it could be the case for remove_if (imagine a list of predicates, and calling remove_if passing one of the predicates in the list). I'll fix this up later.
Reference: SVN commit 215210

zoecarver marked an inline comment as done.Apr 4 2019, 9:25 AM

A meta-question:
Why does list<_Tp, _Alloc>::remove(const value_type& __x) gather all the deleted nodes into a separate list, while
list<_Tp, _Alloc>::remove_if(_Pred __pred) and list<_Tp, _Alloc>::unique() just delete them as they go?

they're all the same operation under the hood.
[ I know you didn't create this difference. ]

Actually very happy you asked about this, I meant to ask the same thing but forgot.

include/list
2172

Forgot size was a member... will do.

zoecarver updated this revision to Diff 193775.Apr 4 2019, 2:02 PM

Use size() member in std::list.

mclow.lists accepted this revision.Jul 1 2019, 12:22 PM

Committed a modified version as revision 364840 (since I changed the implementation of forward_list::remove_if, etc)

This revision is now accepted and ready to land.Jul 1 2019, 12:22 PM
mclow.lists closed this revision.Jul 2 2019, 10:12 PM
miyuki added a subscriber: miyuki.Jul 5 2019, 8:03 AM

Shouldn't the type change be guarded with a conditional compilation construct? I could write

std::list<int> l
assert(typeid(void) == typeid(l.unique()));

and expect this assertion to pass when compiling with, say, -std=c++98.

Shouldn't the type change be guarded with a conditional compilation construct? I could write

std::list<int> l
assert(typeid(void) == typeid(l.unique()));

and expect this assertion to pass when compiling with, say, -std=c++98.

Yes, it should. I'll fix it. Thanks!

Fixed list in revision 365261

Fixed forward_list in revision 365290