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

Stabilize num_midpoint_signed feature #134340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Dec 15, 2024

This PR proposes that we stabilize the signed variants of iN::midpoint, the operation is equivalent to doing (a + b) / 2 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 signed integral type. This implies that the result is
/// always rounded towards zero and that no overflow will ever occur.

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

T-libs-api previously stabilized the unsigned (and float) variants in #131784, the signed variants were left out because of the rounding that should be used in case of negative midpoint.

This stabilization proposal proposes that we round towards zero because:

  • it makes the obvious (a + b) / 2 in a sufficiently-large number always true
    • using another rounding for the positive result would be inconsistent with the unsigned variants
  • it makes midpoint(-a, -b) == -midpoint(a, b) always true
  • it is consistent with midpoint(a as f64, b as f64) as i64
  • it makes it possible to always suggest midpoint as a replacement for (a + b) / 2 expressions (which we may want to do as a future work given the 21.2k hits on GitHub Search)

@scottmcm mentioned a drawback in #132191 (comment):

I'm torn, because rounding towards zero makes it "wider" than other values, which >> 1 avoids -- (a + b) >> 1 has the nice behaviour that midpoint(a, b) + 2 == midpoint(a + 2, b + 2).

But I guess overall sticking with (a + b) / 2 makes sense as well, and I do like the negation property 🤷

Which I think is outweigh by the advantages cited above.

Closes #110840
cc @rust-lang/libs-api
cc @scottmcm
r? @dtolnay

@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 Dec 15, 2024
@Urgau Urgau mentioned this pull request Dec 15, 2024
7 tasks
@Urgau Urgau added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 15, 2024
@dtolnay
Copy link
Member

dtolnay commented Dec 15, 2024

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 15, 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 Dec 15, 2024
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 15, 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. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

Tracking Issue for num_midpoint
4 participants