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

inlined_vector: Use trivial relocation for SwapInlinedElements #1618

Closed
wants to merge 1 commit into from

Conversation

Quuxplusone
Copy link
Contributor

I noticed while working on #1615 that inlined_vector could use the trivial relocatability trait here, too.
Here the memcpy codepath already exists; we just have to opt in to using it.

@derekmauro
Copy link
Member

Going to try to import this now. I don't expect any issues.

@Quuxplusone
Copy link
Contributor Author

Awesome, 92c8575 LGTM!
FWIW, re this comment:

// TODO(absl-team): Using unique_ptr here is technically correct, but
// a trivially relocatable struct would be less semantically confusing.
TEST(UniquePtr, Swap)

notice that we definitely don't want to use a trivially copyable struct, because that wouldn't sufficiently test the behavior we're trying to test. I assume the tests in the UniquePtr group use unique_ptr heap-allocation because we can easily valgrind the tests to double-check that each element's value is destroyed once (not twice or zero times). At least I hope it's true that you can easily (and nightly) valgrind the tests. If not, I'd be the first to agree that all this stuff is woefully under-tested! :)

@Quuxplusone Quuxplusone deleted the trivial-swap branch February 21, 2024 17:36
@derekmauro
Copy link
Member

FWIW, re this comment:

// TODO(absl-team): Using unique_ptr here is technically correct, but
// a trivially relocatable struct would be less semantically confusing.
TEST(UniquePtr, Swap)

notice that we definitely don't want to use a trivially copyable struct, because that wouldn't sufficiently test the behavior we're trying to test. I assume the tests in the UniquePtr group use unique_ptr heap-allocation because we can easily valgrind the tests to double-check that each element's value is destroyed once (not twice or zero times). At least I hope it's true that you can easily (and nightly) valgrind the tests. If not, I'd be the first to agree that all this stuff is woefully under-tested! :)

A code reviewer suggested that comment, but you are correct. Maybe it would be better to static_assert that the unique_ptr has the properties that we want for testing as a form of self documentation, along with a comment about why we choose that type. We don't have valgrind testing, but we do have MSAN testing, which (I think?) is just as good.

netkex pushed a commit to netkex/abseil-cpp that referenced this pull request Apr 3, 2024
…edElements`

Imported from GitHub PR abseil#1618

I noticed while working on abseil#1615 that `inlined_vector` could use the trivial relocatability trait here, too.
Here the memcpy codepath already exists; we just have to opt in to using it.
Merge 567a1dd into a7012a5

Merging this change closes abseil#1618

COPYBARA_INTEGRATE_REVIEW=abseil#1618 from Quuxplusone:trivial-swap 567a1dd
PiperOrigin-RevId: 609019296
Change-Id: I4055ab790245752179e405b490fcd479e7389726
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.

None yet

3 participants