Skip to content

Add method for computing the difference of two unsigned integers in a signed #117476

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

Closed
davidzeng0 opened this issue Nov 1, 2023 · 3 comments
Closed
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR.

Comments

@davidzeng0
Copy link
Contributor

fn overflowing_difference_signed(a: unsigned, b: unsigned) -> (signed, bool);

This is useful in the case of

async fn seek(&mut self, seek: SeekFrom) -> Result<u64> {
  match seek {
    SeekFrom::Current(pos) => self.seek_relative(pos).await,
    SeekFrom::End(pos) => ...
    SeekFrom::Start(pos: u64) => {
      if !self.stream_position_fast() {
        return self.seek_inner(seek).await;
      }

      let stream_pos: u64 = self.stream_position().await?;
 
      /* want to make sure pos - stream_pos can fit in an i64 without overflow */
      /* stream_pos = u64::MAX, pos = 0, seek_relative = -1: wrong */
      self.seek_relative(pos.wrapping_sub(stream_pos) as i64).await
    }
  }
}

This is better than casting u64s to u128s and doing try_into i64 as u128s cannot be upgraded to u256s

GCC and Clang have this feature via

__builtin_sub_overflow(a, b, &output)

which generates the following code: https://godbolt.org/z/E64GnaEz6

This can be implemented in rust with sub_check2: https://godbolt.org/z/dxrP4d9vT
which has zero branches for optimal performance

sub_large: 1.029584449 ns / rep
sub_check: 1.436846155 ns / rep
sub_check2: 1.02297698 ns / rep

CPU: AMD R7 5800X

New code:

let (pos: i64, overflow: bool) = pos.overflowing_difference_signed(stream_pos);

if overflow {
  self.seek_inner(seek).await
} else {
  self.seek_relative(pos)
}
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 1, 2023
@quaternic
Copy link

Another alternative implementation:

pub fn overflowing_signed_difference(a: u64, b: u64) -> (i64,bool) {
    // Translate the unsigned range 0..=u64::MAX
    // to the signed range i64::MIN..=i64::MAX
    let a = (a as i64).wrapping_add(i64::MIN);
    let b = (b as i64).wrapping_add(i64::MIN);
    // The difference is preserved
    a.overflowing_sub(b)
}

For consistency with existing methods like i64::overflowing_sub_signed(i64, u64) -> (i64, bool), I believe that name is clearer (the difference is signed, as opposed to subtracting a signed number). Even then it could be confusing.

@davidzeng0
Copy link
Contributor Author

davidzeng0 commented Nov 1, 2023

I believe that name is clearer

I agree. It should instead be named overflowing_signed_difference

@saethlin saethlin added C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR.
Projects
None yet
Development

No branches or pull requests

4 participants