Skip to content

Stabilize unsigned and float variants of num_midpoint feature #131784

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

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Oct 16, 2024

This PR proposes that we stabilize the unsigned variants of the num_midpoint feature as well as the floats variants, since they are not subject to any unresolved questions, which is equivalent to doing (a + b) / 2 (and (a + b) >> 1) in a sufficiently large number.

The stabilized API surface would be:

/// Calculates the middle point of `self` and `rhs`.
///
/// `midpoint(a, b)` is `(a + b) / 2` as if it were performed in a sufficiently-large unsigned integral type.
/// This implies that the result is always rounded towards negative infinity and that no overflow will ever occur.

impl u{8,16,32,64,128,size} {
    pub const fn midpoint(self, rhs: Self) -> Self;
}

impl NonZeroU{8,16,32,64,size} {
    pub const fn midpoint(self, rhs: Self) -> Self;
}

impl f{32,64} {
    pub const fn midpoint(self, rhs: Self) -> Self;
}

The signed variants u{8,16,32,64,128,size} would remain gated, until a decision is made about the rounding mode, in other words that the unresolved questions are resolved.

cc @rust-lang/libs-api
cc @scottmcm
r? libs-api

@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. labels Oct 16, 2024
@Urgau Urgau mentioned this pull request Oct 16, 2024
7 tasks
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Oct 16, 2024
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 16, 2024
@dtolnay
Copy link
Member

dtolnay commented Oct 16, 2024

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 16, 2024

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 16, 2024
@dtolnay dtolnay added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2024
@scottmcm
Copy link
Member

This absolutely is 👍 from me. (Thanks for the ping!)

I'm a fan of it as described here where it's consistent with (a + b) / 2, like the discussion of this in https://devblogs.microsoft.com/oldnewthing/20220207-00/?p=106223 . I like midpoint(5, 10) == 7 == midpoint(10, 5). To me, being consistent with what people wrote for small values that are in-range is good, and that it being symmetric is also good.

That said, I feel obliged to mention that in the tracking issue that some people want midpoint(5, 10) == 7 but midpoint(10, 5) == 8, where it's consistent with a + (b - a) / 2 instead, so wanted to make sure that libs-api discusses the rounding explicitly.

@scottmcm
Copy link
Member

Actually, @Urgau , consider putting "and float" in the title & OP here. To me the "stabilize the unsigned variants" text wouldn't include floats, since they're obviously signed.

@Urgau Urgau changed the title Stabilize unsigned variants of num_midpoint feature Stabilize unsigned variants and floats of num_midpoint feature Oct 16, 2024
@Urgau Urgau changed the title Stabilize unsigned variants and floats of num_midpoint feature Stabilize unsigned and float variants of num_midpoint feature Oct 16, 2024
@tgross35
Copy link
Contributor

Float midpoint should probably get a const gate, either here or separately. Bit unfortunate that it doesn't already have one to get stabilized at the same time.

@Urgau Urgau force-pushed the stabilize-midpoint branch from 388f953 to 1345744 Compare October 17, 2024 14:13
@Urgau
Copy link
Member Author

Urgau commented Oct 17, 2024

Float midpoint should probably get a const gate, either here or separately.

Good point, updated the PR to include the const stabilization of f{32,64}::midpoint since there is nothing preventing it.

@joshtriplett
Copy link
Member

That said, I feel obliged to mention that in the tracking issue that some people want midpoint(5, 10) == 7 but midpoint(10, 5) == 8, where it's consistent with a + (b - a) / 2 instead, so wanted to make sure that libs-api discusses the rounding explicitly.

I would advocate the behavior that a.midpoint(b) == b.midpoint(a) for all a and b.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2024
…r=dtolnay

Round negative signed integer towards zero in `iN::midpoint`

This PR changes the implementation of `iN::midpoint` (the signed variants) to round negative signed integers **towards zero** *instead* of negative infinity as is currently the case.

This is done so that the obvious expectations[^1] of `midpoint(a, b) == midpoint(b, a)` and `midpoint(-a, -b) == -midpoint(a, b)` are true, which makes the even more obvious implementation `(a + b) / 2` always true.

The unsigned variants `uN::midpoint` (which are being [FCP-ed](rust-lang#131784 (comment))) already rounds towards zero, so there is no consistency issue.

cc `@scottmcm`
r? `@dtolnay`

[^1]: rust-lang#110840 (comment)
@bors

This comment was marked as resolved.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 27, 2024
@Urgau Urgau force-pushed the stabilize-midpoint branch 2 times, most recently from 70e5e08 to e217d60 Compare October 27, 2024 13:31
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 30, 2024
@Urgau Urgau force-pushed the stabilize-midpoint branch from cf3aa1d to b88478f Compare December 1, 2024 10:37
@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2024

@bors r=dtolnay

@bors
Copy link
Collaborator

bors commented Dec 1, 2024

📌 Commit b88478f has been approved by dtolnay

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 1, 2024
@bors bors merged commit 5880752 into rust-lang:master Dec 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.