- Notifications
You must be signed in to change notification settings - Fork14.1k
Resolve Clippyf16 andf128unimplemented!/FIXMEs#126636
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
rustbot commentedJun 18, 2024
Some changes occurred in src/tools/clippy cc @rust-lang/clippy These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
tgross35 commentedJun 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
@rustbot label +F-f16_and_f128 |
64443c7 to26ef017Compare| 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); | ||
| }, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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 justf.to_bits().hash(state);. I left a fixme here instead, maybe we could convert everything to f128 once that is better supported.
There was a problem hiding this comment.
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 usef.to_bits().hash(state); everywhere. But that's not for this PR to decide. Let's leave the FIXME here and address this later.
| clippy::identity_op | ||
| )] | ||
| // FIXME(f16_f128): add tests once const casting is available for these types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
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 thestandard library for a while.
26ef017 to477e9e8Comparetgross35 commentedJun 19, 2024
I added a couple more FIXMEs in places where
|
flip1995 commentedJun 20, 2024
Thanks for the summary what the current road map for this feature is! Let's get this in to unblock you. @bors r+ |
bors commentedJun 20, 2024
…flip1995Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`sThis was originally a PR against the Clippy repo,rust-lang/rust-clippy#12950r? `@flip1995`Tracking issue:rust-lang#116909Fixes:rust-lang#126636
…iaskrgrRollup of 7 pull requestsSuccessful 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
Rollup merge ofrust-lang#126636 - tgross35:clippy-f16-f128-fixme, r=flip1995Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`sThis was originally a PR against the Clippy repo,rust-lang/rust-clippy#12950r? ``@flip1995``Tracking issue:rust-lang#116909Fixes:rust-lang#126636
tgross35 commentedJun 20, 2024
Thank you for the review! |
Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`sThis was originally a PR against the Clippy repo,rust-lang#12950r? ``@flip1995``Tracking issue:rust-lang/rust#116909Fixes:rust-lang/rust#126636
Add `f16` and `f128` math functionsWIPThis will be pretty finicky among selection failures, the lowering bug, and lack of symbols. But might as well start somewhere.Checks will fail untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
Uh oh!
There was an error while loading.Please reload this page.
This was originally a PR against the Clippy repo,rust-lang/rust-clippy#12950
r?@flip1995
Tracking issue:#116909
Fixes:#126636