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

Fix clippy warnings. #88

Closed
wants to merge 10 commits into from
Closed

Conversation

Frostie314159
Copy link
Contributor

This also removes multiple uses of transmute, which have safe equivalents. All tests pass and clippy is happy.

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a great initial cleanup, thank you so much! only question was on the one transmute, but it looks fine to me. otherwise i think we have to merge your other PR and fix ci in it before we merge this one?

src/ctx.rs Show resolved Hide resolved
src/ctx.rs Outdated Show resolved Hide resolved
src/ctx.rs Outdated Show resolved Hide resolved
@nyurik
Copy link
Contributor

nyurik commented Nov 5, 2023

I fixed the CI - MSRV in #90

@m4b
Copy link
Owner

m4b commented Nov 13, 2023

so if you're having trouble fixing this, you can do:

git checkout master
git pull
git checkout clippy-fix # assumes this is your branch name
git rebase master
# you will likely have to manually fix the conflict markers (choose the 0x4a case)
git add src/lib.rs
git rebase --continue
git push --force # force push the rebased change to your branch 

@nyurik
Copy link
Contributor

nyurik commented Nov 13, 2023

@m4b and @Frostie314159 I created a PR for this PR - fixing merge conflicts. It should be merged in before merging this PR - Frostie314159#1

Merge with main, fixing merge conflicts
@m4b
Copy link
Owner

m4b commented Nov 13, 2023

@nyurik it still says there is a merge conflict?

@m4b
Copy link
Owner

m4b commented Nov 13, 2023

Also, i'm not sure what's going on, but i'm having trouble reviewing the "final changes" of this PR; when i choose to review all commits, only 3 files are changed, and some changes that were supposedly removed (the try_from_ctx impl for [u8; N]) is still present, and all the transmute and safety fixes are missing. 😕
review_screenshot

I'd expect reviewing all changes to show me what i'm going to merge, maybe i haven't done this in a while and i my memory doesn't serve, but i don't remember github presenting stacks of commits in this manner? (i remember being able to view what he entire set of changes introduced, which is what i want to see, and which is what would be merged into master)

@nyurik
Copy link
Contributor

nyurik commented Nov 13, 2023

@m4b those fixes were also done and merged in by my earlier PR. This PR only has the stuff that my (also clippy-inspired PR) did not have. The diff tool (add /compare to your repo) will help us explore this:

Main branch has two commits: f5beda1 and 30f49c7. The second commit is mostly irrelevant (format inlining), so comparing with it is pointless because it will just increase the noise. The first commit on the other hand is the one that contains a lot of the changes that were also done by this commit.

You can see it by comparing it with 62c5fcf:
https://github.com/m4b/scroll/compare/f5beda1c9..62c5fcf -- this is the commit right before my PR was merged into the @Frostie314159 fork.

Now the very next commit - caf60bb:

Next is the merge commit -- eac19ef -- merge commit is a noop for us, so same results as before

So in short - all transmute fixes are already in the main branch.

@nyurik
Copy link
Contributor

nyurik commented Nov 13, 2023

P.S. the conflict is tiny - I fixed it in Frostie314159#2 -- this is again just a simple PR that was created with git merge upstream/master and fixing conflicts.

-- this PR modifies two lines: this and this - both in src/lib.rs - and my last PR modifies println!("1 {:?}", &hello_world); which is right above those lines. So git requires manual check when two adjacent lines are modified by two separate commits.

Merge in the main branch and fixes last remaining conflicts
@Frostie314159
Copy link
Contributor Author

Thank you so much!

Comment on lines +486 to +488
bytes_to.iter_mut().enumerate().for_each(|(i, item)| {
*item = bytes_from.pread(i).unwrap();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is a bit hard to read - iterating over the "target" as mutable. I would suggest a simpler variant (i didn't test it, might not even compile) -- iterate over source values, pushing them into the output array.

Suggested change
bytes_to.iter_mut().enumerate().for_each(|(i, item)| {
*item = bytes_from.pread(i).unwrap();
});
for (idx, value) in bytes_from.iter().enumerate() {
bytes_to[i] = value.unwrap();
});

bytes_to[i] = bytes_from.gread(&mut offset).unwrap();
}
let offset = &mut 0;
bytes_to.iter_mut().for_each(|item| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although this might need to be different - not certain about this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the clippy suggestion.

@@ -136,6 +137,11 @@ impl<'a> Segments<'a> {
Ok(sections)
}
}
impl<'a> Default for Segments<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? You can simply add #[derive(Default)] on the Segments struct, and make new() call Self::default(). Also, I am not too certain if both default() and new() are needed. I think cleaner interface would be to have just one. This way if later you need new(some_init_value), it can be easily added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I can get to this tomorrow.

@m4b m4b force-pushed the master branch 2 times, most recently from 18d11bc to d369188 Compare January 1, 2024 00:12
@m4b
Copy link
Owner

m4b commented Jan 1, 2024

i believe all these fixes have been merged, if not we should do this in another PR since this has conflicts, etc.

@m4b m4b closed this Jan 1, 2024
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

Successfully merging this pull request may close these issues.

3 participants