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

Re-add From<f16> for f64 #124728

Merged
merged 1 commit into from
May 16, 2024
Merged

Re-add From<f16> for f64 #124728

merged 1 commit into from
May 16, 2024

Conversation

beetrees
Copy link
Contributor

@beetrees beetrees commented May 4, 2024

This impl was originally added in #122470 before being removed in #123830 due to #123831. However, the issue only affects f32 (which currently only has one From<{float}> impl, From<f32>) as f64 already has two From<{float}> impls (From<f32> and From<f64>) and is also the float literal fallback type anyway. Therefore it is safe to re-add From<f16> for f64.

This PR also updates the FIXME link to point to the open issue #123831 rather than the closed issue #123824.

Tracking issue: #116909

@rustbot label +F-f16_and_f128 +T-libs-api

@rustbot
Copy link
Collaborator

rustbot commented May 4, 2024

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 4, 2024
@joboet
Copy link
Member

joboet commented May 5, 2024

r? libs-api

@rustbot rustbot assigned BurntSushi and unassigned joboet May 5, 2024
@BurntSushi
Copy link
Member

I agree with the reasoning here, but I'd like to be cautious and do a crater run. Since this is an unstable API, r=me if the crater run doesn't reveal anything.

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
Re-add `From<f16> for f64`

This impl was originally added in rust-lang#122470 before being removed in rust-lang#123830 due to rust-lang#123831. However, the issue only affects `f32` (which currently only has one `From<{float}>` impl, `From<f32>`) as `f64` already has two `From<{float}>` impls (`From<f32>` and `From<f64>`) and is also the float literal fallback type anyway. Therefore it is safe to re-add `From<f16> for f64`.

This PR also updates the FIXME link to point to the open issue rust-lang#123831 rather than the closed issue rust-lang#123824.

Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128 +T-libs-api
@bors
Copy link
Contributor

bors commented May 5, 2024

⌛ Trying commit 5cc4ee3 with merge 98ebbae...

@bors
Copy link
Contributor

bors commented May 5, 2024

☀️ Try build successful - checks-actions
Build commit: 98ebbae (98ebbae35138252af77610c20ea5a5e8bf68d639)

@BurntSushi
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-124728 created and queued.
🤖 Automatically detected try build 98ebbae
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2024
@tgross35
Copy link
Contributor

tgross35 commented May 6, 2024

Just FYI I removed this intentionally since it seemed inconsistent to have Into one of the existing float types but not the other. I have no significant qualms about adding it back though.

@beetrees
Copy link
Contributor Author

beetrees commented May 6, 2024

I just thought it seemed inconsistent not to have it since From<f16> for f128 still exists. IMO the distinction between existing and new floating point types is ideally one that should disappear over time as the new floating point types will hopefully be eventually supported just as well as the existing ones.

@craterbot
Copy link
Collaborator

🚧 Experiment pr-124728 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-124728 is completed!
📊 1 regressed and 2 fixed (446250 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 16, 2024
@beetrees
Copy link
Contributor Author

Only regression is spurious:

[INFO] [stderr]     Updating crates.io index
[INFO] [stderr] error: failed to download `assert_matches v1.5.0`
[INFO] [stderr] 
[INFO] [stderr] Caused by:
[INFO] [stderr]   unable to get packages from source
[INFO] [stderr] 
[INFO] [stderr] Caused by:
[INFO] [stderr]   attempting to make an HTTP request, but --frozen was specified

@BurntSushi
Copy link
Member

Okey dokey. Good enough for me. @bors r+

@bors
Copy link
Contributor

bors commented May 16, 2024

📌 Commit 5cc4ee3 has been approved by BurntSushi

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 May 16, 2024
@bors
Copy link
Contributor

bors commented May 16, 2024

⌛ Testing commit 5cc4ee3 with merge 2d89cee...

@bors
Copy link
Contributor

bors commented May 16, 2024

☀️ Test successful - checks-actions
Approved by: BurntSushi
Pushing 2d89cee to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 16, 2024
@bors bors merged commit 2d89cee into rust-lang:master May 16, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 16, 2024
@beetrees beetrees deleted the from-f16-for-f64 branch May 16, 2024 18:59
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2d89cee): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.8%, secondary -4.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.8% [2.8%, 2.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.3% [-4.3%, -4.3%] 1
All ❌✅ (primary) 2.8% [2.8%, 2.8%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.731s -> 682.198s (0.66%)
Artifact size: 316.21 MiB -> 316.23 MiB (0.01%)

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)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants