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

Use assoc int and float consts instead of module level ones #5429

Merged
merged 8 commits into from
Apr 8, 2020

Conversation

faern
Copy link
Contributor

@faern faern commented Apr 6, 2020

changelog: Recommend primitive type associated constants instead of module level constants

In Rust 1.43 integer and float primitive types will have a number of new associated constants. For example MAX, MIN and a number of constants related to the machine representation of floats. rust-lang/rust#68952

These new constants are preferred over the module level constants in {core,std}::{f*, u*, i*}. I have in the last few days made sure that the documentation in the main rust repository uses the new constants in every place I could find (rust-lang/rust#69860, rust-lang/rust#70782). So the next step is naturally to make the linter recommend the new constants as well.

This PR only changes two lints. There are more. But I did not want the PR to be too big. And since I have not contributed to clippy before it felt saner to start with a small PR so I see if there are any quirks. More will come later.

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 6, 2020
@faern faern force-pushed the use-assoc-int-float-consts branch from 346c7d7 to 7f1405a Compare April 6, 2020 23:23
@faern faern changed the title Use assoc int float consts Use assoc int float consts for NAN and EPSILON Apr 6, 2020
@flip1995
Copy link
Member

flip1995 commented Apr 6, 2020

Thanks! If you want to, you can also update the rest of the constants in the Clippy code base. For things like this, the size of the PR doesn't really matter. Splitting this up in small commits, like you did is great for reviewing though.

7f1405a overlaps with #5345.

@flip1995
Copy link
Member

flip1995 commented Apr 6, 2020

The failing CI runs are due to a bug in GHA, so don't mind them

@flip1995
Copy link
Member

flip1995 commented Apr 6, 2020

@faern Do you want to update more constants in this PR or do this in later PRs? I'm good with both.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 6, 2020
@faern
Copy link
Contributor Author

faern commented Apr 6, 2020

7f1405a overlaps with #5345.

That's another reason I don't want to submit a ton in a single PR. It usually ends up with collisions from other stuff that is merged faster and then it's a hell of rebasing over and over again. usually simpler to get small stuff in quicker and have more of them IMO.

@faern
Copy link
Contributor Author

faern commented Apr 6, 2020

So I'd rather have this merged and move on to the other stuff. If that's fine with you. Either way I'm done coding for today so anything else will have to come another day.

@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 6, 2020
@flip1995
Copy link
Member

flip1995 commented Apr 6, 2020

Alright, waiting for GHA to get fixed.

@faern faern changed the title Use assoc int float consts for NAN and EPSILON Use assoc int and float consts instead of module level ones Apr 7, 2020
@faern
Copy link
Contributor Author

faern commented Apr 7, 2020

I'll see if I can make some more of my dirty working directory into commits that suit in this PR. Starting with these two extra commits. Let's see if CI likes them. I can't run the tests locally. Or any help with how to even do that would be appreciated.

Any update on the GHA status?

@faern faern force-pushed the use-assoc-int-float-consts branch from f1f9db7 to 1647f53 Compare April 7, 2020 22:43
@faern
Copy link
Contributor Author

faern commented Apr 7, 2020

Sweet. It passes! Now everything I had laying around is pushed to this PR. So it ended up being one large PR with all my changes anyway after all. Hopefully these can be merged before the PR starts experiencing bit rot.

@flip1995
Copy link
Member

flip1995 commented Apr 8, 2020

Thanks! Prioritizing this PR before the big rollup.

@bors r+ p=10

@bors
Copy link
Contributor

bors commented Apr 8, 2020

📌 Commit 1647f53 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Apr 8, 2020

🌲 The tree is currently closed for pull requests below priority 2, this pull request will be tested once the tree is reopened

@bors
Copy link
Contributor

bors commented Apr 8, 2020

⌛ Testing commit 1647f53 with merge 0b40983...

@bors
Copy link
Contributor

bors commented Apr 8, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 0b40983 to master...

@bors bors merged commit 0b40983 into rust-lang:master Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants