Conversation
rslawson
force-pushed
the
rs/clippy-lints
branch
2 times, most recently
from
bbf6a5b to
2977b52
Compare
Okay, so. You can set up clippy lints in `Cargo.toml` to catch some extra
things that might not be included by default in the lint group you're using.
We've discussed wanting to allow a couple things/disallow a couple others that
aren't in the default lint group, so I've done these things:
1. Added those lints into the workspace `Cargo.toml`s
2. Inherited those lints in the individual crates' `Cargo.toml`s
3. Adjusted code to respect these new lints and remove now-unnecessary
`#[allow(clippy::<something>)]`s
4. Adjusted the rest of the code where it was simple to remove any other
`#[allow(clippy::<something>)]`s. If it was a simple rewrite of something
I immediately understood, then I made the change. Otherwise, I left it
alone.
rslawson
force-pushed
the
rs/clippy-lints
branch
from
2977b52 to
d63d3c5
Compare
| instruction_memory: u32::MAX..0, | ||
| data_memory: u32::MAX..0, | ||
| instruction_memory: 0..0, | ||
| data_memory: 0..0, |
There was a problem hiding this comment.
I don't understand this change. Is this a result of linting rules?
There was a problem hiding this comment.
It's because I deleted the #[allow(clippy::reversed_empty_ranges)] just above.
There was a problem hiding this comment.
Owww! Good guy clippy.
There was a problem hiding this comment.
Below the code uses start = start.min(seg.addr) to calculate the bounds, this doesn't work if the start is 0!
So this edit isn't quite right.
There was a problem hiding this comment.
As I wrote, this changes the behaviours slightly. It will always increase the size of the memories. Nothing should access this memory, and the memory-mapped regions will still be in the right spots, but it means the reporting of the sizes of memories will not be right. Not sure why that's not caught by the test actually, but I think this should be reverted.
hydrolarus
left a comment
hydrolarus
left a comment
There was a problem hiding this comment.
Overall looking good, just the elf-loading test should be reverted
| dut :: | ||
| (HasCallStack) => | ||
| Circuit (ToConstBwd Mm) (Df System (BitVector 8), CSignal System (ClockControlData LinkCount)) | ||
| dut = |
There was a problem hiding this comment.
If you made this
| dut :: | |
| (HasCallStack) => | |
| Circuit (ToConstBwd Mm) (Df System (BitVector 8), CSignal System (ClockControlData LinkCount)) | |
| dut = | |
| dut :: | |
| (HasCallStack) => | |
| Circuit (ToConstBwd Mm, ()) (Df System (BitVector 8), CSignal System (ClockControlData LinkCount)) | |
| dut = |
then below you could use getMMAny to get the memory map and unMemmap to get a circuit without the memory map, meaning less juggling of functions and Circuit wrappers
| instruction_memory: u32::MAX..0, | ||
| data_memory: u32::MAX..0, | ||
| instruction_memory: 0..0, | ||
| data_memory: 0..0, |
There was a problem hiding this comment.
As I wrote, this changes the behaviours slightly. It will always increase the size of the memories. Nothing should access this memory, and the memory-mapped regions will still be in the right spots, but it means the reporting of the sizes of memories will not be right. Not sure why that's not caught by the test actually, but I think this should be reverted.
| impl<T> IntoIterator for Storage<T> { | ||
| type Item = (Handle<T>, T); | ||
| type IntoIter = | ||
| std::iter::Map<std::iter::Enumerate<std::vec::IntoIter<T>>, fn((usize, T)) -> Self::Item>; | ||
|
|
||
| fn into_iter(self) -> Self::IntoIter { | ||
| self.0 | ||
| .into_iter() | ||
| .enumerate() | ||
| .map(|(idx, val)| (Handle(idx, PhantomData), val)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
cool! Yeah I meant to do this but thought I'd have to use a custom type, using the std types and fn is cool!
I've been wondering if this module should be turned into a crate at some point, I've wanted this for compiler development multiple times before 🙈
3 participants
Okay, so. You can set up clippy lints in
Cargo.tomlto catch some extrathings that might not be included by default in the lint group you're using.
We've discussed wanting to allow a couple things/disallow a couple others that
aren't in the default lint group, so I've done these things:
Cargo.tomlsCargo.tomls#[allow(clippy::<something>)]s#[allow(clippy::<something>)]s. If it was a simple rewrite of something I immediately understood, then I made the change. Otherwise, I left it alone.Closes #1035