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

enable clippy lints against integer casts #2422

Merged
merged 3 commits into from
Jul 25, 2022
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 23, 2022

Cc #1236

src/shims/mod.rs Outdated
@@ -1,3 +1,5 @@
#![warn(clippy::integer_arithmetic)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how useful this one is... none of he cases are caught, so we could just as well turn on debug asserts in release mode if we haven't already. Would be more readable than the checked math functions

Copy link
Member Author

Choose a reason for hiding this comment

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

turn on debug asserts in release mode

Any suggestion for how to do that in a way that also applies to the builds produced by rustc? We cannot use cargo profiles.

Also we do have a bunch of arithmetic outside this folder (stacked borrows, data races, weak memory) that I deliberately did not enable this lint for, so there might be a performance difference. But that should be easy to benchmark.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can change Bootstrap's build flags for miri

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I don't expect this to make a difference. It's mostly in math heavy code where this is visible in practice

Copy link
Member Author

Choose a reason for hiding this comment

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

We can change Bootstrap's build flags for miri

How? RUSTFLAGS affects all dependencies so it'll rebuild everything...

I am going to move the contentious part into a separate PR.

@RalfJung
Copy link
Member Author

@oli-obk I hope the remaining changes are uncontroversial. :)

@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2022

They are

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2022

📌 Commit 7f60348 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 25, 2022

⌛ Testing commit 7f60348 with merge 83d50ef...

bors added a commit that referenced this pull request Jul 25, 2022
enable clippy lints against integer casts and arithmetic

Cc #1236
@RalfJung RalfJung changed the title enable clippy lints against integer casts and arithmetic enable clippy lints against integer casts Jul 25, 2022
@RalfJung
Copy link
Member Author

@bors retry

I fixed the PR title

@bors
Copy link
Contributor

bors commented Jul 25, 2022

⌛ Testing commit 7f60348 with merge 6227e1e...

@bors
Copy link
Contributor

bors commented Jul 25, 2022

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

@bors bors merged commit 6227e1e into rust-lang:master Jul 25, 2022
@RalfJung RalfJung deleted the integers branch July 25, 2022 16:43
bors added a commit that referenced this pull request Aug 22, 2022
pass clippy::integer_arithmetic in our shims

`@oli-obk` [raised some concerns](#2422 (comment)) 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
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