Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow vector to be assigned to itself #84

Closed
wants to merge 1 commit into from

Conversation

Timmmm
Copy link

@Timmmm Timmmm commented Sep 11, 2018

This sounds like an odd thing to do but some implementations of std::random_shuffle() do this because they call std::swap(x, x) (and cppreference implies that they are allowed to). Old versions of libstdc++ used to do this but now they don't. The current as-of-now version of libc++ does it for one variant of std::random_shuffle() but not others. I have opened a bug for them to fix that, but swap(a, a) should still be allowed anyway.

Quoting cppreference:

C++ named requirements: Swappable
Any lvalue or rvalue of this type can be swapped with any lvalue or rvalue of some other type

Also relevant SO question

This sounds like an odd thing to do but some implementations of `std::random_shuffle()` do this because they call `std::swap(x, x)` (and cppreference implies that they are allowed to). Old versions of libstdc++ used to do this but now they don't. The current as-of-now version of libc++ does it for one variant of `std::random_shuffle()` but not others. I have opened [a bug](https://bugs.llvm.org/show_bug.cgi?id=38900) for them to fix that, but `swap(a, a)` should still be allowed anyway.

Quoting [cppreference](https://en.cppreference.com/w/cpp/named_req/Swappable):

> C++ named requirements: Swappable
> Any lvalue or rvalue of this type can be swapped with any lvalue or rvalue of some other type
@igaztanaga
Copy link
Member

igaztanaga commented Nov 10, 2018

I can't see why std::swap should fail, as it would use a temporary vector to do the move assignment.

Edit: I see the assertion would fire. But I don't like the idea of pessimizing the vector move operation with a branch. Maybe I should simply allow it with empty vectors.

@igaztanaga
Copy link
Member

I've added a commit with some changes:

2bc6f4d

-> Assertion is changed to allow empty self-moving, which is typical when self swap is made through std::swap.
-> priv_swap allows explicit self-swapping.

Let me know if this change avoids assertions in your use case.

@Timmmm
Copy link
Author

Timmmm commented Nov 12, 2018

This might fix std::swap(x, x) however I think you misunderstood the bug report - this comment is wrong:

   //x.size() == 0 is allowed for buggy std libraries.

This bug report is not about buggy standard libraries. This is a bug in boost that is exposed by an unusual (but perfectly allowed) standard library. The following should work, but I don't think it will with your change.

boost::container::vector<int> x;
x.push_back(5);
x = std::move(x);

Hope that's clear!

@igaztanaga
Copy link
Member

I was about to reply that self-movement need not to be supported at all, but this seems to have changed in C++17 (where MoveAssignable requirement has a note about self-assignment), but in C++11 any self move-assignment is undefined behavior. This change is reflected also in a SO question and Howard Hinnant's reply and update:

https://stackoverflow.com/questions/13129031/on-implementing-stdswap-in-terms-of-move-assignment-and-move-constructor

Boost.Container implemented C++11 guarantees, maybe it's time to update to C++17 requirements.

@igaztanaga
Copy link
Member

Opened #88 for a general review of self move assignments.

@igaztanaga
Copy link
Member

Closing this as #88 was fixed allowing vector to assign to itself. Thanks for the report.

@igaztanaga igaztanaga closed this Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants