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

pass clippy::integer_arithmetic in our shims #2441

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 25, 2022

@oli-obk raised some concerns about this one. I still think it is the right call, since I don't see a good way to enable overflow checks for our official release builds. I'm open to suggestions though!

Fixes #1236

@bors
Copy link
Contributor

bors commented Jul 25, 2022

☔ The latest upstream changes (presumably #2422) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 29, 2022

I looked into it and I see no reason why we can't just add debug-assertions=true to the [profile.release] section of our Cargo.toml.

@RalfJung
Copy link
Member Author

We just recently stopped using profiles because they dont work in workspaces - only the profile in the workspace root Cargo.toml is taken into account.

@bors
Copy link
Contributor

bors commented Aug 6, 2022

☔ The latest upstream changes (presumably #2470) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung RalfJung added the S-waiting-on-review Status: Waiting for a review to complete label Aug 17, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2022

well.. it would mean we at least run with debug assertions if built from within the miri repo, but not when building via x.py. We would then have to tell bootstrap separately to build the miri crate with debug assertions.

@RalfJung
Copy link
Member Author

It also means showing a warning about profiles being ignored on each and every cargo invocation done by x.py. That's not really an option IMO.

@RalfJung
Copy link
Member Author

We would then have to tell bootstrap separately to build the miri crate with debug assertions.

That is the build we ship to users, so it is the most important one here. If that build doesn't end up with overflow checks, we might as well not change anything.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2022

.cargo/config.toml to the rescue? Anyway... while I dislike moving to manually checked arithmetic, I do see that it's annoying to ensure we actually generate checked arithmetic, so let's go with this PR for now.

@RalfJung
Copy link
Member Author

.cargo/config.toml to the rescue?

I don't know how to use that to instruct bootstrap to build the Miri crate with overflow checks, but not anything else. Usually flags are the same for an entire workspace, aren't they?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2022

Bootstrap will need its own separate logic, but if we had .cargo/config.toml inside this repo, bootstrap would ignore it and only local builds would pick it up

@RalfJung
Copy link
Member Author

RalfJung commented Aug 22, 2022

Bootstrap will need its own separate logic, but if we had .cargo/config.toml inside this repo, bootstrap would ignore it and only local builds would pick it up

I am not trying to solve a problem with local builds. We can use RUSTFLAGS for that, and anyway CI uses debug builds for ./miri test so we have overflow checks there.

I am trying to solve overflow checks for the build we ship to users.

@RalfJung
Copy link
Member Author

let's go with this PR for now.

Thanks!
@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Aug 22, 2022

📌 Commit 8497fd4 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 22, 2022

⌛ Testing commit 8497fd4 with merge 6e306f9...

@RalfJung
Copy link
Member Author

Opened #2501 to track finding a more elegant solution here.

@bors
Copy link
Contributor

bors commented Aug 22, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6e306f9 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use clippy to avoid unchecked casts and arithmetic
3 participants