Conversation
|
As noted in #60, I can compile |
152d35e to
7e7f407
Compare
|
@nicoburns Sorted out the MSRV, the CI failures, and hkBst's comments. |
| _len: SizeType, | ||
| _cap: SizeType, | ||
| len: SizeType, | ||
| cap: SizeType, |
There was a problem hiding this comment.
Given that this crate is designed to support FFI with an equivalent vectors type in Gecko's C++ code, I think we may need to keep the field names as they were. It's probably possible to change them here with an change to Gecko's bindgen code, but also probably not worth the bother?
Can we just ignore this lint?
There was a problem hiding this comment.
No need to do this, the names are already different in Gecko.
| final_size as usize | ||
| final_size | ||
| .try_into() | ||
| .expect("cannot allocate object larger than isize") |
There was a problem hiding this comment.
Use of try_into for these casts seems like it could cause a perf regression? Which seems a bit unnecessary given that we can be confident that they will always succeed.
Do you have a motivation for changing them other than that the lint says to?
There was a problem hiding this comment.
Yeah I'd leave these casts as they are.
| _len: SizeType, | ||
| _cap: SizeType, | ||
| len: SizeType, | ||
| cap: SizeType, |
There was a problem hiding this comment.
No need to do this, the names are already different in Gecko.
| final_size as usize | ||
| final_size | ||
| .try_into() | ||
| .expect("cannot allocate object larger than isize") |
There was a problem hiding this comment.
Yeah I'd leave these casts as they are.
| /// ``` | ||
| pub fn dedup(&mut self) { | ||
| self.dedup_by(|a, b| a == b) | ||
| self.dedup_by(|a, b| a == b); |
There was a problem hiding this comment.
These changes also feel pretty random.
|
Would be good to split this PR (or the commits at least) in formatting changes, then style changes (which shouldn't have perf impact) then others. |
|
Okay, I don't have the free time for now to work on that. Anyone is free to take up the mantle either from scratch or based on this PR. |
4 participants
Currently some warn-by-default lints fire in the tests, this PR fixes up the existing warnings, then adds pedantic and some nursery lints in for good luck.