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

Style: replace Box<Arc<T>> with Arc<T> #72

Open
daladim opened this issue Nov 14, 2022 · 1 comment
Open

Style: replace Box<Arc<T>> with Arc<T> #72

daladim opened this issue Nov 14, 2022 · 1 comment

Comments

@daladim
Copy link
Collaborator

daladim commented Nov 14, 2022

That's what is suggested by Clippy

warning: usage of `Box<Arc<CallbackData>>`
   --> src\trace.rs:173:20
    |
173 |     callback_data: Box<Arc<CallbackData>>,
    |                    ^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(clippy::redundant_allocation)]` on by default
    = note: `Arc<CallbackData>` is already on the heap, `Box<Arc<CallbackData>>` makes an extra allocation
    = help: consider using just `Box<CallbackData>` or `Arc<CallbackData>`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation

In our case, we do not only want the CallbackData to be on the heap, but the ref counter as well.
https://github.com/rust-lang/rust/blob/96ddd32c4bfb1d78f0cd03eb068b1710a8cebeef/library/alloc/src/sync.rs#L352 suggests the ref counters (both atomic::usizes) really live on the heap, so this change should be sensible.

That's to avoid an allocation, on a struct that's only created a handful of times, this currently properly works that way, and it's a private implementation detail of the crate (so it can be changed at anytime without any trouble for the users), so I'm leaving this for later.

@daladim
Copy link
Collaborator Author

daladim commented Nov 14, 2022

Currently, I like the fact that Box<Arc<...>> really makes the intent clear (the Arc is on a fixed location on the heap). Maybe that's a good enough reason to keep it this way.

daladim added a commit to daladim/ferrisetw that referenced this issue Nov 14, 2022
See n4r1b#72.
It probably may be simplified to Arc<T>, but this would make the intent less explicit
daladim added a commit to daladim/ferrisetw that referenced this issue Dec 2, 2022
See n4r1b#72.
It probably may be simplified to Arc<T>, but this would make the intent less explicit
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

No branches or pull requests

1 participant