tree-buf
tree-buf copied to clipboard
That3Percent
Clippy lints
not sure this case is covered by the contribution guidelines.
this pull request represents a bunch of small, largely cosmetic refactors -the vast majority of which are clippy lints.
weirdly, there's quite a few formatting changes too, despite the presence of .rustfmt.toml file in the crate root. Either rustfmt hasn't been run for a while, or perhaps it's not usually run with the nightly compiler?
update: closes #28
Thanks for the contribution! I really appreciate it.
Most of this is positive changes, but I need to give it a more thorough review before merging. Skimming the code, there are a couple of issues that stand out as potential problems that will come up during review that I'll just give the heads up on now...
First, there were some removals of calls to drop. I'm not sure if/why clippy suggested this - but this a breaking change for the profiler which is using drop to signify the end of a region. I suspect that this is coming up because firestorm compiles to a no-op when not enabled and clippy may have noticed that if that's the case then it's returning () or similar... I'll need to make a change to firestorm to fix that.
The call to skip may have performance implications. If you look at the source, it would be moving a check that is basically if(i == 0) { continue } to the inside of the loop, rather than just starting at 1 as the code does now. The compiler may optimize that away, I haven't checked, but I tend to be distrustful of these kinds of things.
The rest looks pretty good but I'll give a more thorough review soon.
I was torn between splitting this into separate pull requests and filling your inbox, and having a monolithic mess to go through.
No performance regression with cargo bench, but I don't know how naive a test that is.
The linter is complaining about the drop call not doing anything on a Copy type. See https://doc.rust-lang.org/std/mem/fn.drop.html.
I thought the import deglobbing might be controversial. I guess it's a matter of taste
Mixing multiple changes in one PR is usually OK by me. I think allowing that ends up with a better result overall because splitting up changes as you're making edits as you go can be such a hassle as to discourage the edits.
For the import de-globbing, I really don't care. At all. I think there is a tendency to waste a lot of hot air on things that are superficial - and the entire conversation detracts from getting good at the things that actually matter.
there are some functions with unused parameters. Some of these are in trait implementations (including self), so i'm assuming you have these as placeholders for other implementors. There's one or two non-trait implementations with unused arguments too, what's the reason for this?
examples include
-
tree_buf::internal::types::integer::BytesCompressor::new -
tree_buf::internal::buffer::Buffer::capacity
(there might be others)
Making some progress here... I just did the first release of Firestorm (the profiler dependency). In this new version, we shouldn't get the lints for dropping the profiling sections since the macro will return a non-Copy type now even when profiling is disabled.
Making some progress here... I just did the first release of Firestorm (the profiler dependency). In this new version, we shouldn't get the lints for dropping the profiling sections since the macro will return a non-Copy type now even when profiling is disabled.
I've reinstated the drop calls. still flagging the linter though
would love to add
#![deny(clippy::all)]
#![warn(clippy::pedantic)]
but that's going to give you something like 50 warnings/errors right now
