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

Address clippy lints #75

Merged
merged 5 commits into from
Jun 25, 2024
Merged

Address clippy lints #75

merged 5 commits into from
Jun 25, 2024

Conversation

caass
Copy link
Contributor

@caass caass commented Jun 14, 2024

Clippy threw some lints, so I just fixed them. In most cases, this was pretty straightforward. One lint was a little more involved: in LockFreeFrozenVec, I added a // ## Safety comment that mirrored the comments elsewhere in the codebase:

/// Load an element (if it exists). This operation is lock-free and
/// performs no synchronized atomic operations. This is a useful primitive to
/// implement your own data structure with newtypes around the index.
///
/// ## Safety
///
/// `index` must be in bounds, i.e. it must be less than `self.len()`
#[inline]
pub unsafe fn get_unchecked(&self, index: usize) -> T {
    let buffer_idx = buffer_index(index);
    let buffer_ptr = self.data[buffer_idx].load(Ordering::Relaxed);
    let local_index = index - prior_total_buffer_size(buffer_idx);
    unsafe { *buffer_ptr.add(local_index) }
}

However, a LockFreeFrozenVec::len method didn't exist, so I wrapped the existing calls to self.len.load(Ordering::Acquire):

#[inline(always)]
pub fn len(&self) -> usize {
    self.len.load(Ordering::Acquire)
}

This modified the code in LockFreeFrozenVec::is_empty, which previously used Ordering::Relaxed. However, since stores always use Ordering::Release, there is no visible behavior change.

src/sync.rs Outdated
@@ -704,15 +704,18 @@ impl<T: Copy> Default for LockFreeFrozenVec<T> {
/// any heap allocations until the first time data is pushed to it.
fn default() -> Self {
Self {
data: [Self::NULL; NUM_BUFFERS],
data: std::array::from_fn(|_| Self::null()),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can just use [Self::null(); NUM_BUFFERS] here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we can't use [Self::null; NUM_BUFFERS] because AtomicPtr doesn't impl Copy. The workaround is to define some const (i.e., what happened before clippy said anything). With the new const context syntax it's possible to do [const { Self::null() }; NUM_BUFFERS] instead, which is what I did in the second commit.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I'd just allow the lint? I think the const is fine, and from_fn isn't guaranteed const so I'd rather not use it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative is to do

impl<T: Copy> LockFreeFrozenVec<T> {
    const fn null() -> [AtomicPtr<T>; NUM_BUFFERS] {
        [const { AtomicPtr::new(std::ptr::null_mut()) }; NUM_BUFFERS]
    }
}

And replace calls to [Self::NULL; NUM_BUFFERS] with Self::null()

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me!

src/sync.rs Show resolved Hide resolved
src/sync.rs Outdated Show resolved Hide resolved
src/sync.rs Show resolved Hide resolved
@caass
Copy link
Contributor Author

caass commented Jun 19, 2024

It looks like the GHA runners don't have Rust 1.79 installed yet. Maybe adding a rust-version.toml that pins to stable?

rust-toolchain.toml Outdated Show resolved Hide resolved
@Manishearth Manishearth merged commit ab1beef into Manishearth:master Jun 25, 2024
1 check passed
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