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

Resolve clippy warnings in normal build #157

Closed
wants to merge 3 commits into from
Closed

Resolve clippy warnings in normal build #157

wants to merge 3 commits into from

Conversation

zrneely
Copy link

@zrneely zrneely commented Dec 25, 2019

These changes fix a few cases where byteorder invokes undefined behavior (caught by clippy) by dereferencing a possibly unaligned pointer (see here).

While I was doing that, I additionally cleaned up the other clippy warnings around the use of the try! macro and using ptr::offset instead of ptr::add.

IMO the only important changes here are the fixes to the UB, so I'm happy to revert the other changes and open a different PR with them (or just forget about them), if you'd prefer.

Edit: I should have looked at the Travis config - looks like 1.12 is the minimum Rust version, so I've reverted my proposed changes around use of try!.

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Wow great find! I wonder though if we can perhaps do this without bumping the MSRV?

If we do have to bump the MSRV, then I'm okay with that, since Rust 1.12 is quite old by this point.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@BurntSushi
Copy link
Owner

Thanks! I merged this manually because the commits were not organized. In the process, I forgot to carry over your author info. Sorry. :-(

@zrneely zrneely deleted the clippy_fixes branch January 11, 2020 20:01
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.

2 participants