n4r1b / ferrisetw

Basically a KrabsETW rip-off written in Rust
Other
65 stars 24 forks source link

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

Open daladim opened 2 years ago

daladim commented 2 years ago

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 commented 2 years ago

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.