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.

Sign up for GitHub

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

Jump to bottom

New lints [manual_is_infinite] and [manual_is_finite] #11049

Merged
merged 3 commits into from Jul 8, 2023
Merged

New lints [manual_is_infinite] and [manual_is_finite] #11049

merged 3 commits into from Jul 8, 2023

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jun 30, 2023 •
edited by xFrednet

Closes #9665

changelog: New lints [manual_is_infinite] and [manual_is_finite]
#11049

@rustbot
Copy link
Collaborator

rustbot commented Jun 30, 2023

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 30, 2023
@bors
Copy link
Collaborator

bors commented Jul 1, 2023

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

@asquared31415
Copy link
Contributor

x != <float>::INFINITY && x != <float>::NEG_INFINITY returns true for NaN where is_finite returns false.

@Centri3
Copy link
Member Author

Centri3 commented Jul 1, 2023 •
edited

Almost nobody would intentionally account for NaN in those cases regardless. Should add a note about || x.is_nan(), though, and change to MaybeIncorrect

@Centri3
Copy link
Member Author

Centri3 commented Jul 1, 2023 •
edited

We could probably suggest either !x.is_infinite() or x.is_finite() in that case, depending on what they wanted (the original code's behavior or the "fixed" version, respectively). Though x.is_infinite() || x.is_nan() may be more readable than !x.is_infinite()

@xFrednet
Copy link
Member

xFrednet commented Jul 1, 2023

r ? @blyxyas

Would you mind taking over this review :)

@xFrednet
Copy link
Member

xFrednet commented Jul 1, 2023

BTW, one thing you might also want to check, is if this is in a const block. I would guess that x != <float>::NEG_INFINITY works in const, while still unstable in const :)

@blyxyas
Copy link
Member

blyxyas commented Jul 1, 2023

Yeah, I will take over and control this review. Subjugate it

@Centri3
Copy link
Member Author

Centri3 commented Jul 2, 2023

BTW, one thing you might also want to check, is if this is in a const block. I would guess that x != <float>::NEG_INFINITY works in const, while still unstable in const :)

Good point, we can probably check that and make sure const_float_classify is not enabled

blyxyas
blyxyas reviewed Jul 5, 2023
View reviewed changes
clippy_lints/src/manual_float_methods.rs Show resolved Hide resolved
tests/ui/manual_float_methods.rs Show resolved Hide resolved
clippy_lints/src/manual_float_methods.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_float_methods.rs Outdated Show resolved Hide resolved
@Centri3 Centri3 force-pushed the manual_is_infinite branch 2 times, most recently from 49e864f to fa31c15 Compare July 6, 2023 19:24
blyxyas
blyxyas approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! ❤️
cc @xFrednet

xFrednet
xFrednet approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@xFrednet xFrednet 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 good to me, just two small nits and then it's ready for bors. Thank you for the implementation!

And thank you @blyxyas for the review :D

clippy_lints/src/manual_float_methods.rs Outdated Show resolved Hide resolved
tests/ui/manual_float_methods.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Jul 7, 2023

You can r=blyxyas,xFrednet after the update :)

Lord bors, I hereby allow queen @Centri3 to merge this PR into the master kingdom, in the name of princess @blyxyas and me ^^

@bors
Copy link
Collaborator

bors commented Jul 7, 2023

✌️ @Centri3, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=@xFrednet

@Centri3
Copy link
Member Author

Centri3 commented Jul 8, 2023

@bors r=blyxyas,xFrednet

@bors
Copy link
Collaborator

bors commented Jul 8, 2023

📌 Commit 41438c2 has been approved by blyxyas,xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 8, 2023

⌛ Testing commit 41438c2 with merge 8e261c0...

@bors
Copy link
Collaborator

bors commented Jul 8, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas,xFrednet
Pushing 8e261c0 to master...

@bors bors merged commit 8e261c0 into rust-lang:master Jul 8, 2023
8 checks passed
@Centri3 Centri3 deleted the manual_is_infinite branch July 8, 2023 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet approved these changes

@blyxyas blyxyas blyxyas approved these changes

Assignees

@xFrednet xFrednet

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest {f32, f64}::is_finite and {f32, f64}::is_infinite
6 participants
@Centri3 @rustbot @bors @asquared31415 @xFrednet @blyxyas