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 f16 and f128 unimplemented!/FIXMEs #126636

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 18, 2024

This was originally a PR against the Clippy repo, rust-lang/rust-clippy#12950

r? @flip1995

Tracking issue: #116909
Fixes: #126636

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

rustbot commented Jun 18, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@tgross35
Copy link
Contributor Author

tgross35 commented Jun 18, 2024

I know you suggested to include this as part of #126608, but it may as well just get its own PR since it touches a lot of files.

I need some help understanding https://github.com/rust-lang/rust-clippy/actions/runs/9569119207/job/26380925098?pr=12950#step:5:1055, I am not sure why it looks like it just deletes the file. I just forgot to add the feature gates

@rustbot label +F-f16_and_f128

@rustbot rustbot added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Jun 18, 2024
Comment on lines 170 to 182
Self::F16(f) => {
f64::from(f).to_bits().hash(state);
},
Self::F32(f) => {
f64::from(f).to_bits().hash(state);
},
Self::F64(f) => {
f.to_bits().hash(state);
},
Self::F128(f) => {
f.to_bits().hash(state);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part could still use a double check, not sure if there is something better to do here

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we go from f32 to f64 before hashing. But keeping this consistent for f16 seems reasonable 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume it is to make sure there is a hash that depends only on the value and not the size, which , but I can't find anywhere that it is being used.

Unfortunately, I forgot that conversions to/from f16 aren't yet present on all platforms so I had to change this to just f.to_bits().hash(state);. I left a fixme here instead, maybe we could convert everything to f128 once that is better supported.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we could convert everything to f128 once that is better supported.

Might be a good idea, yes. But I wonder how much that matters of if we should just use f.to_bits().hash(state); everywhere. But that's not for this PR to decide. Let's leave the FIXME here and address this later.

@@ -17,6 +17,8 @@
clippy::identity_op
)]

// FIXME(f16_f128): add tests once const casting is available for these types
Copy link
Member

@flip1995 flip1995 Jun 19, 2024

Choose a reason for hiding this comment

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

Can those tests be added with #126608 being merged? If so, after merging this PR, please add those in #126608

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#126608 won't add casting, but it unblocks a followup PR that does add it. I'll make sure to add the Clippy tests with that one.

src/tools/clippy/tests/ui/cast_size.rs Outdated Show resolved Hide resolved
This removes the ICE codepaths for `f16` and `f128` in Clippy.
`rustc_apfloat` is used as a dependency for the parsing of these types,
since their `FromStr` implementation will not be available in the
standard library for a while.
@tgross35
Copy link
Contributor Author

I added a couple more FIXMEs in places where Constant::{F32, F64} is checked nonexhaustively but can't yet be added because it relies on math. @flip1995 to make things a bit more clear, here is my loose plan:

@flip1995
Copy link
Member

Thanks for the summary what the current road map for this feature is!

Let's get this in to unblock you.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2024

📌 Commit 477e9e8 has been approved by flip1995

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 20, 2024
…flip1995

Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`s

This was originally a PR against the Clippy repo, rust-lang/rust-clippy#12950

r? `@flip1995`

Tracking issue: rust-lang#116909
Fixes: rust-lang#126636
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#126380 (Add std Xtensa targets support)
 - rust-lang#126636 (Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`s )
 - rust-lang#126659 (More status-quo tests for the `#[coverage(..)]` attribute)
 - rust-lang#126711 (Make Option::as_[mut_]slice const)
 - rust-lang#126717 (Clean up some comments near `use` declarations)
 - rust-lang#126719 (Fix assertion failure for some `Expect` diagnostics.)
 - rust-lang#126730 (Add opaque type corner case test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors closed this in d3f5e7b Jun 20, 2024
@bors bors merged commit d3f5e7b into rust-lang:master Jun 20, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2024
Rollup merge of rust-lang#126636 - tgross35:clippy-f16-f128-fixme, r=flip1995

Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`s

This was originally a PR against the Clippy repo, rust-lang/rust-clippy#12950

r? ``@flip1995``

Tracking issue: rust-lang#116909
Fixes: rust-lang#126636
@tgross35
Copy link
Contributor Author

Thank you for the review!

@tgross35 tgross35 deleted the clippy-f16-f128-fixme branch June 20, 2024 18:28
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`s

This was originally a PR against the Clippy repo, rust-lang#12950

r? ``@flip1995``

Tracking issue: rust-lang/rust#116909
Fixes: rust-lang/rust#126636
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
Add `f16` and `f128` math functions

WIP

This will be pretty finicky among selection failures, the lowering bug, and lack of symbols. But might as well start somewhere.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants