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

Implement strict integer operations that panic on overflow #116090

Merged
merged 4 commits into from
Jan 21, 2024

Conversation

rmehri01
Copy link
Contributor

@rmehri01 rmehri01 commented Sep 23, 2023

This PR implements the first part of the ACP for adding panic on overflow style arithmetic operations (rust-lang/libs-team#270), mentioned in #116064.

It adds the following operations on both signed and unsigned integers:

  • strict_add
  • strict_sub
  • strict_mul
  • strict_div
  • strict_div_euclid
  • strict_rem
  • strict_rem_euclid
  • strict_neg
  • strict_shl
  • strict_shr
  • strict_pow

Additionally, signed integers have:

  • strict_add_unsigned
  • strict_sub_unsigned
  • strict_abs

And unsigned integers have:

  • strict_add_signed

The div and rem operations are the same as normal division and remainder but are added for completeness similar to the corresponding wrapping_* operations.

I'm not sure if I missed any operations, I basically found them from the wrapping_* and checked_* operations on both integer types.

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@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 Sep 23, 2023
@asquared31415
Copy link
Contributor

All of the exposed methods should probably be #[track_caller] to improve the panic locations.

@bors
Copy link
Contributor

bors commented Sep 29, 2023

☔ The latest upstream changes (presumably #116176) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 1, 2023

☔ The latest upstream changes (presumably #117482) made this pull request unmergeable. Please resolve the merge conflicts.

@eduardosm
Copy link
Contributor

cc @RalfJung (I didn't know if you were aware of this PR)

/// #![feature(strict_overflow_ops)]
#[doc = concat!("let _ = (", stringify!($SelfT), "::MAX - 2).strict_add(3);")]
/// ```
#[unstable(feature = "strict_overflow_ops", issue = "116064")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably better to create a new tracking issue for this, with the tracking issue template.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I created #118260.

///
/// ## Overflow behavior
///
/// This function will always panic on overflow, regardless of if overflow checks are enabled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// This function will always panic on overflow, regardless of if overflow checks are enabled.
/// This function will always panic on overflow, regardless of whether overflow checks are enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be useful to add a comment to this file to note that these functions are used by the strict_ methods.

Copy link
Member

@m-ou-se m-ou-se Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RalfJung How would you feel about removing the descriptions from rustc_middle and instead of calling the panic lang item, making each of the functions below a lang item and calling those? (In a follow-up PR.)

Copy link
Member

@RalfJung RalfJung Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think description is used in a bunch of places where we really need a string, e.g. for MIR dumping.

But if there's some way to reduce the redundancy here, I'm all for it. This duplication of the panic message in 3 or 4 places has bothered me for a while, I just never found a great way to fix it.

#[track_caller]
pub const fn strict_add(self, rhs: Self) -> Self {
let (a, b) = self.overflowing_add(rhs);
if unlikely!(b) {overflow_panic::add()} else {a}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if unlikely!(b) {overflow_panic::add()} else {a}
if unlikely!(b) { overflow_panic::add() } else { a }

@m-ou-se m-ou-se 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2023
@rmehri01
Copy link
Contributor Author

rmehri01 commented Dec 2, 2023

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 2, 2023
@RalfJung
Copy link
Member

@m-ou-se just a reminder that there's a PR waiting for review here :)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 19, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 19, 2024

📌 Commit 6d17169 has been approved by m-ou-se

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 Jan 19, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 20, 2024
…u-se

Implement strict integer operations that panic on overflow

This PR implements the first part of the ACP for adding panic on overflow style arithmetic operations (rust-lang/libs-team#270), mentioned in rust-lang#116064.

It adds the following operations on both signed and unsigned integers:

- `strict_add`
- `strict_sub`
- `strict_mul`
- `strict_div`
- `strict_div_euclid`
- `strict_rem`
- `strict_rem_euclid`
- `strict_neg`
- `strict_shl`
- `strict_shr`
- `strict_pow`

Additionally, signed integers have:

- `strict_add_unsigned`
- `strict_sub_unsigned`
- `strict_abs`

And unsigned integers have:

- `strict_add_signed`

The `div` and `rem` operations are the same as normal division and remainder but are added for completeness similar to the corresponding `wrapping_*` operations.

I'm not sure if I missed any operations, I basically found them from the `wrapping_*` and `checked_*` operations on both integer types.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#116090 (Implement strict integer operations that panic on overflow)
 - rust-lang#118811 (Use `bool` instead of `PartiolOrd` as return value of the comparison closure in `{slice,Iteraotr}::is_sorted_by`)
 - rust-lang#119081 (Add Ipv6Addr::is_ipv4_mapped)
 - rust-lang#119461 (Use an interpreter in MIR jump threading)
 - rust-lang#119996 (Move OS String implementation into `sys`)
 - rust-lang#120015 (coverage: Format all coverage tests with `rustfmt`)
 - rust-lang#120027 (pattern_analysis: Remove `Ty: Copy` bound)
 - rust-lang#120084 (fix(rust-analyzer): use new pkgid spec to compare)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b661cd6 into rust-lang:master Jan 21, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
Rollup merge of rust-lang#116090 - rmehri01:strict_integer_ops, r=m-ou-se

Implement strict integer operations that panic on overflow

This PR implements the first part of the ACP for adding panic on overflow style arithmetic operations (rust-lang/libs-team#270), mentioned in rust-lang#116064.

It adds the following operations on both signed and unsigned integers:

- `strict_add`
- `strict_sub`
- `strict_mul`
- `strict_div`
- `strict_div_euclid`
- `strict_rem`
- `strict_rem_euclid`
- `strict_neg`
- `strict_shl`
- `strict_shr`
- `strict_pow`

Additionally, signed integers have:

- `strict_add_unsigned`
- `strict_sub_unsigned`
- `strict_abs`

And unsigned integers have:

- `strict_add_signed`

The `div` and `rem` operations are the same as normal division and remainder but are added for completeness similar to the corresponding `wrapping_*` operations.

I'm not sure if I missed any operations, I basically found them from the `wrapping_*` and `checked_*` operations on both integer types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants