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

absl::InlinedVector doesn't support move constructors which throw #14

Closed
jorgbrown opened this issue Sep 28, 2017 · 3 comments
Closed
Assignees

Comments

@jorgbrown
Copy link
Contributor

std::vector, when resizing, will only use move if it is declared nothrow.

absl's InlinedVector uses move operations for resizing, regardless.

This means that if a resize operation calls the move constructor and it throws, we end up with a situation where the vector is only partially moved to its new location. Moving the data back might also throw, and we end up with an inconsistent InlinedVector.

InlinedVector should either use copy constructors in that case, or insist that the move operator is declared nothrow.

@jorgbrown
Copy link
Contributor Author

User reported test case at http://codepad.org/YbVsjMy5 but it seems to be inaccessible now.

@JonathanDCohen
Copy link
Contributor

On a slightly pedantic note, an InlinedVector of objects in maybe-moved-from states is still in a valid state, so this technically satisfies basic exception safety -- but I agree it's not a nice thing to do.

I think this is a two step solution -- we declare that InlinedVector only supports T with nothrow move operators in the short term.

In the long term, Abseil's stance on throwing moves constructors is essentially that move constructors can only throw if the allocator can throw -- we're stricter than the standard on thsi point of view. So as I ramp up the exception-safety test suite we can advise the move constructor to be noexcept if the allocator is noexcept and use the current algorithm, and fall back to copying only if the allocator throws and T has a throwing move constructor.

Does this sound reasonable?

@JonathanDCohen
Copy link
Contributor

Closing as a duplicate of #37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants