Skip to content

Commit

Permalink
Emphasize move semantics isn't needed for cross-kind raw_ptr conversion
Browse files Browse the repository at this point in the history
This CL doesn't change the code, just adds comments and tests.

Cross-kind move will degrade to cross-kind copy, which is what we want,
as different traits may use different ref-count, which a move operation
wouldn't update.

There is a worry that an operation would silently kick off a chain of
implicit conversions: raw_ptr<T, X> -> T* -> raw_ptr<T, Y>, but the
tests confirm that this doesn't happen with a move.

Bug: 1407995
Change-Id: I6adcbe3c699d768283626dcd7ad4dfb26cc54d3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263931
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Sergei Glazunov <glazunov@google.com>
Cr-Commit-Position: refs/heads/main@{#1107673}
  • Loading branch information
bartekn-chromium authored and Chromium LUCI CQ committed Feb 21, 2023
1 parent d3901ef commit 6331eec
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 4 deletions.
6 changes: 6 additions & 0 deletions base/allocator/partition_allocator/pointers/raw_ptr.h
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,9 @@ class PA_TRIVIAL_ABI PA_GSL_POINTER raw_ptr {
#endif // BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) ||
// BUILDFLAG(USE_ASAN_UNOWNED_PTR)

// Cross-kind copy constructor.
// Move is not supported as different traits may use different ref-counts, so
// let move operations degrade to copy, which handles it well.
template <RawPtrTraits PassedTraits,
typename Unused = std::enable_if_t<Traits != PassedTraits>>
PA_ALWAYS_INLINE explicit raw_ptr(const raw_ptr<T, PassedTraits>& p) noexcept
Expand All @@ -853,6 +856,9 @@ class PA_TRIVIAL_ABI PA_GSL_POINTER raw_ptr {
static_assert(Traits == (PassedTraits | RawPtrTraits::kMayDangle));
}

// Cross-kind assignment.
// Move is not supported as different traits may use different ref-counts, so
// let move operations degrade to copy, which handles it well.
template <RawPtrTraits PassedTraits,
typename Unused = std::enable_if_t<Traits != PassedTraits>>
PA_ALWAYS_INLINE raw_ptr& operator=(
Expand Down
11 changes: 7 additions & 4 deletions base/allocator/partition_allocator/pointers/raw_ptr_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1360,14 +1360,15 @@ TEST_F(RawPtrTest, CrossKindConversion) {
RawPtrCountingMayDangleImpl::ClearCounters();

CountingRawPtrMayDangle<int> ptr2(ptr1);
CountingRawPtrMayDangle<int> ptr3(std::move(ptr1)); // Falls back to copy.

EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingImpl>{
.get_for_dereference_cnt = 0,
.get_for_extraction_cnt = 0,
.get_for_duplication_cnt = 1}),
.get_for_duplication_cnt = 2}),
CountersMatch());
EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingMayDangleImpl>{
.wrap_raw_ptr_cnt = 0, .wrap_raw_ptr_for_dup_cnt = 1}),
.wrap_raw_ptr_cnt = 0, .wrap_raw_ptr_for_dup_cnt = 2}),
CountersMatch());
}

Expand All @@ -1379,15 +1380,17 @@ TEST_F(RawPtrTest, CrossKindAssignment) {
RawPtrCountingMayDangleImpl::ClearCounters();

CountingRawPtrMayDangle<int> ptr2;
CountingRawPtrMayDangle<int> ptr3;
ptr2 = ptr1;
ptr3 = std::move(ptr1); // Falls back to copy.

EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingImpl>{
.get_for_dereference_cnt = 0,
.get_for_extraction_cnt = 0,
.get_for_duplication_cnt = 1}),
.get_for_duplication_cnt = 2}),
CountersMatch());
EXPECT_THAT((CountingRawPtrExpectations<RawPtrCountingMayDangleImpl>{
.wrap_raw_ptr_cnt = 0, .wrap_raw_ptr_for_dup_cnt = 1}),
.wrap_raw_ptr_cnt = 0, .wrap_raw_ptr_for_dup_cnt = 2}),
CountersMatch());
}

Expand Down
30 changes: 30 additions & 0 deletions base/allocator/partition_allocator/pointers/raw_ptr_unittest.nc
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,20 @@ void WontCompile() {
[[maybe_unused]] raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr2(ptr);
}

#elif defined(NCTEST_CROSS_KIND_MOVE_CONVERSION_FROM_MAY_DANGLE) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)0U == \(\(base::RawPtrTraits\)1U | RawPtrTraits::kMayDangle\)'"]

void WontCompile() {
raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr = new int(3);
[[maybe_unused]] raw_ptr<int> ptr2(std::move(ptr));
}

#elif defined(NCTEST_CROSS_KIND_MOVE_CONVERSION_FROM_DUMMY) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)0U == \(\(base::RawPtrTraits\)1U | RawPtrTraits::kMayDangle\)'"]

void WontCompile() {
raw_ptr<int, base::RawPtrTraits::kDummyForTest> ptr = new int(3);
[[maybe_unused]] raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr2(std::move(ptr));
}

#elif defined(NCTEST_CROSS_KIND_ASSIGNMENT_FROM_MAY_DANGLE) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)1U == \(\(base::RawPtrTraits\)16U | RawPtrTraits::kMayDangle\)'"]

void WontCompile() {
Expand All @@ -257,6 +271,22 @@ void WontCompile() {
ptr2 = ptr;
}

#elif defined(NCTEST_CROSS_KIND_MOVE_ASSIGNMENT_FROM_MAY_DANGLE) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)1U == \(\(base::RawPtrTraits\)16U | RawPtrTraits::kMayDangle\)'"]

void WontCompile() {
raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr = new int(3);
raw_ptr<int> ptr2;
ptr2 = std::move(ptr);
}

#elif defined(NCTEST_CROSS_KIND_MOVE_ASSIGNMENT_FROM_DUMMY) // [r"static assertion failed due to requirement '\(base::RawPtrTraits\)1U == \(\(base::RawPtrTraits\)16U | RawPtrTraits::kMayDangle\)'"]

void WontCompile() {
raw_ptr<int, base::RawPtrTraits::kDummyForTest> ptr = new int(3);
raw_ptr<int, base::RawPtrTraits::kMayDangle> ptr2;
ptr2 = std::move(ptr);
}

#endif

} // namespace

0 comments on commit 6331eec

Please sign in to comment.