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

Simplify Clone by removing redundant guards #458

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

JustForFun88
Copy link
Contributor

@JustForFun88 JustForFun88 commented Aug 24, 2023

These extra guards only complicate the code where it is not needed anyway. The Drop function checks the number of items in a table before dropping elements (see below), so that dropping the uninitialize table as well as table with no actual data (but with FULL control bytes) is safe . Added tests to check that the current behavior of Drop won't change in the future.

impl<T, A: Allocator + Clone> Drop for RawTable<T, A> {
    fn drop(&mut self) {
        if !self.table.is_empty_singleton() {
            unsafe {
                self.drop_elements();
                self.free_buckets();
            }
        }
    }
}

impl<T, A: Allocator + Clone> RawTable<T, A> {
    unsafe fn drop_elements(&mut self) {
        if Self::DATA_NEEDS_DROP && !self.is_empty() {
            for item in self.iter() {
                item.drop();
            }
        }
    }
}

@JustForFun88
Copy link
Contributor Author

JustForFun88 commented Aug 24, 2023

By the way, the old implementation with ManuallyDrop seems to be leaking the allocator. I'll add a test later

The old implementation with ManuallyDrop actually leaked the allocator. A trifle of course, but still corrected the test to take this into account too.

@Amanieu
Copy link
Member

Amanieu commented Aug 24, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 24, 2023

📌 Commit 50a9e7b has been approved by Amanieu

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 24, 2023

⌛ Testing commit 50a9e7b with merge bb6521e...

@bors
Copy link
Contributor

bors commented Aug 24, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing bb6521e to master...

@bors bors merged commit bb6521e into rust-lang:master Aug 24, 2023
24 checks passed
@JustForFun88 JustForFun88 deleted the simplify-clone branch August 25, 2023 02:42
bors added a commit that referenced this pull request Sep 1, 2023
Fix two bugs in `clone_from` one of which is mine 😅 (was introduced in  #458)

My bad 😅, I totally forgot that we can modify table from another thread, which can result in an invalid `self` table in case if we have a panic during the cloning of elements (we will have an empty table with invalid control bytes to which we can get accessed via `RawIterRange` during parallel iteration or via the `find` function).
This pull request fixes my bug by partially reverting the old implementation of the `clone_from` function.

Also fixes leaking allocator when `self_.buckets() != source.buckets`. We used to rewrite the table (`&mut **self_ as *mut Self).write()`), forgetting that although we freed the old table memory, we forgot about the old allocator (**this behavior was found during testing**). Now, to allocate a new table, we use the old allocator, via `ptr::read(&self_.table.alloc)`.

The latter is worth thinking about. Maybe instead of using the old allocator, it's better to use the new one via `alloc.clone()` and just drop the old one?

**P.S.** Added tests for the cases described above.
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.

3 participants