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

Tracking issue for integer methods for Wrapping #32463

Open
22 of 23 tasks
SimonSapin opened this issue Mar 24, 2016 · 21 comments
Open
22 of 23 tasks

Tracking issue for integer methods for Wrapping #32463

SimonSapin opened this issue Mar 24, 2016 · 21 comments
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

SimonSapin commented Mar 24, 2016

u32 and other primitive integer types implement a number of bit-manipulation methods like rotate_left, but Wrapping<_> does not. At the moment this can be worked around with code like Wrapping(x.0.rotate_left(n)) instead of x.rotate_left(n).

It would be nice to implement:

  • count_ones
  • count_zeroes
  • leading_zeroes
  • trailing_zeroes
  • rotate_left
  • rotate_right
  • swap_bytes
  • from_be (?)
  • from_le (?)
  • to_be
  • to_le
  • pow (?)

Edit: Others added after #32463 (comment)

and maybe other methods, for:

  • Wrapping<u8>
  • Wrapping<u16>
  • Wrapping<u32>
  • Wrapping<u64>
  • Wrapping<usize>
  • Wrapping<i8>
  • Wrapping<i16>
  • Wrapping<i32>
  • Wrapping<i64>
  • Wrapping<isize>

Edit: From #50465

  • Decide on correct behavior for wrapping_next_power_of_two
@srinivasreddy
Copy link
Contributor

I want to take this. Would you review my PR?

@SimonSapin
Copy link
Contributor Author

I can have a look, but I’m not one of the reviewers who can approve PRs for merging into this repository.

@srinivasreddy
Copy link
Contributor

sure that helps. thanks.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 24, 2017
@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 17, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 17, 2017

I would be open to considering a PR that adds these methods.

Phlosioneer added a commit to Phlosioneer/rust that referenced this issue Mar 7, 2018
Wrapping<T> now implements:

count_ones, count_zeros, leading_zeros,
trailing_zeros, rotate_left, rotate_right, swap_bytes, from_be,
from_le, to_be, to_le, and pow

where T is:

u8, u16, u32, u64, usize, i8, i16, i32, i64, or isize.

Docs were written for all these methods, as well as examples. The
examples mirror the ones on u8, u16, etc... for consistency.

Closes rust-lang#32463
kennytm added a commit to kennytm/rust that referenced this issue Mar 10, 2018
…wrapping, r=dtolnay

Implement Integer methods for Wrapping

Wrapping<T> now implements:

count_ones, count_zeros, leading_zeros,
trailing_zeros, rotate_left, rotate_right, swap_bytes, from_be,
from_le, to_be, to_le, and pow

where T is:

u8, u16, u32, u64, usize, i8, i16, i32, i64, or isize.

Docs were written for all these methods, as well as examples. The
examples mirror the ones on u8, u16, etc... for consistency.

Closes rust-lang#32463
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Mar 12, 2018
…wrapping, r=dtolnay

Implement Integer methods for Wrapping

Wrapping<T> now implements:

count_ones, count_zeros, leading_zeros,
trailing_zeros, rotate_left, rotate_right, swap_bytes, from_be,
from_le, to_be, to_le, and pow

where T is:

u8, u16, u32, u64, usize, i8, i16, i32, i64, or isize.

Docs were written for all these methods, as well as examples. The
examples mirror the ones on u8, u16, etc... for consistency.

Closes rust-lang#32463
Phlosioneer added a commit to Phlosioneer/rust that referenced this issue Mar 19, 2018
Wrapping<T> now implements:

count_ones, count_zeros, leading_zeros,
trailing_zeros, rotate_left, rotate_right, swap_bytes, from_be,
from_le, to_be, to_le, and pow

where T is:

u8, u16, u32, u64, usize, i8, i16, i32, i64, or isize.

Docs were written for all these methods, as well as examples. The
examples mirror the ones on u8, u16, etc... for consistency.

Closes rust-lang#32463
kennytm added a commit to kennytm/rust that referenced this issue Mar 19, 2018
…wrapping, r=dtolnay

Implement Integer methods for Wrapping

Wrapping<T> now implements:

count_ones, count_zeros, leading_zeros,
trailing_zeros, rotate_left, rotate_right, swap_bytes, from_be,
from_le, to_be, to_le, and pow

where T is:

u8, u16, u32, u64, usize, i8, i16, i32, i64, or isize.

Docs were written for all these methods, as well as examples. The
examples mirror the ones on u8, u16, etc... for consistency.

Closes rust-lang#32463
@clarfonthey
Copy link
Contributor

This is a tracking issue, so, it shouldn't be closed.

Also, a quick search yields that we are missing:

  • min_value and max_value (mostly useful for macros)
  • from_str_radix (changes behaviour)
  • reverse_bits (added after initial PR)
  • abs (missing; should call wrapping_abs)
  • signum(presumably missed, as this is specific to signed integers)
  • is_positive and is_negative (same as above)

@SimonSapin SimonSapin reopened this Mar 26, 2018
@SimonSapin SimonSapin added B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. and removed C-feature-accepted Category: A feature request that has been accepted pending implementation. labels Mar 26, 2018
@SimonSapin SimonSapin changed the title Integer methods for Wrapping Tracking issue for integer methods for Wrapping Mar 26, 2018
@fbstj
Copy link
Contributor

fbstj commented Mar 27, 2018

is it worth changing the top post into a checklist to track completion? since this is a tracking issue.

@scottmcm
Copy link
Member

I guess now mod_euc and div_euc need to go on the list too? #49389

@varkor
Copy link
Member

varkor commented May 21, 2018

Bit-shifting would be useful to have here too.

bors added a commit that referenced this issue May 28, 2018
Add missing Wrapping methods, use doc_comment!

Re-opened version of #49393 . Finishing touches for #32463.

Note that this adds `Shl` and `Shr` implementations for `Wrapping<i128>` and `Wrapping<u128>`, which were previously missed. This is technically insta-stable, but I don't know why this would be a problem.
@briansmith
Copy link

I would like to propose that all of the ones that have been implemented already be stabilized, and that we separate the ones that haven't been implemented into a separate feature. Otherwise stabilization is likely to lose many races with adding new methods to integer types that then should be added to Wrapping.

@bstrie
Copy link
Contributor

bstrie commented May 17, 2019

Any movement on the semantics of next_power_of_two for wrapping types? There are comments for this scattered around other threads, but it would be good if someone on the lib team could summarize the current thinking.

@clarfonthey
Copy link
Contributor

clarfonthey commented May 17, 2019

I strongly believe that wrapping_next_power_of_two should be 0 on overflow. If we do not treat integer types as a truncation of the number line, and instead treat them as proper cyclic monoids, then 0 is a power of two.

@clarfonthey
Copy link
Contributor

Adding the other argument: if you see integers as infinite, rather than finite, zero is never a power of two; therefore, the wrapping version should wrap to one. It's been argued that there is a way to make both versions as performant as one another (and hence the argument is just over preference of the result) but I don't actually know the code for this case. (Maybe @nagisa can comment on that.)

@nagisa
Copy link
Member

nagisa commented Jun 1, 2019

I consider Wrapping<T> types to be (mod 2^b) where b is the number of bits in T. With that wrapping_next_power_of_two should be 0 after an overflow (2^(n*b) = 0 (mod 2^b) for all integer n).

As for implementation, since the of next_power_of_two is mostly bit-fiddling to achieve T::max_value().wrapping_next_power_of_two() = 1 one would have to replace semantic multiplication by 2 with the previous power of 2 with a rotation left by a single position – both equally cheap operations.

@jonhoo
Copy link
Contributor

jonhoo commented Apr 3, 2020

Maybe this is too much, but given that we're implementing the same methods for a bunch of different Ts for Wrapping<T>, would it be worthwhile extracting the relevant methods out into a trait? In my case for example, I'd like to be generic over "any number type that can wrap", and unless I'm missing something, I don't think that's currently doable (except by adding a trait and impls myself).

@clarfonthey
Copy link
Contributor

Pre-1.0 Rust had a Num trait that was explicitly removed in favour of inherent methods, and so far that's what we've stuck with. If you're looking for those traits, the num crates have your back.

@jonhoo
Copy link
Contributor

jonhoo commented Apr 6, 2020

I was specifically thinking of a Wrapping trait for this set of methods, not a general Num trait, though I see the similarities. It was mostly an idle observation though, and I will leave it to those much more familiar with the innards to decide what makes sense :)

@jplatte
Copy link
Contributor

jplatte commented Jun 30, 2020

leading_ones and trailing_ones were stabilized for the regular integer types, but don't even exist for Wrapping (whereas leading_zeros and trailing_zeros do). They should probably be on the list too.

@KodrAus KodrAus added Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@JohnBSmith
Copy link

There is a further argument for wrapping_next_power_of_two to be zero on overflow. One might use

m.wrapping_next_power_of_two().wrapping_sub(1)

to obtain the bitmask of all numbers in 0..m.

@scottmcm
Copy link
Member

... to be zero on overflow

Note that next_power_of_two is already stable as wrapping to 0 in release mode, so I think it would be very surprising for wrapping_next_power_of_two to do anything different. (Regardless of which answer is actually better.)

... to obtain the bitmask of ...

If people want that, I would encourage them to propose it as a separate method. It's nicer than next_power_of_two since it has no overflow hazards, so doesn't need the full checked/wrapping/saturating set.

Strawman for a bikeshed: fill_lower_bits.

@mjbshaw
Copy link
Contributor

mjbshaw commented Mar 17, 2021

Is there still debate around the behavior of wrapping_next_power_of_two? I think that next_power_of_two evaluates to 0 (in release mode) on overflow pretty much requires that wrapping_next_power_of_two does the same; it'd be insanely surprising to behave differently.

If there is a debate around this, how can I get involved to push for stabilization?

tspiteri added a commit to tspiteri/rust that referenced this issue May 11, 2021
This keeps `Wrapping` synchronized with the primitives it wraps as for
the rust-lang#32463 `wrapping_int_impl` feature.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue May 13, 2021
add BITS associated constant to core::num::Wrapping

This keeps `Wrapping` synchronized with the primitives it wraps as for the rust-lang#32463 `wrapping_int_impl` feature.
@gendx
Copy link

gendx commented Jul 21, 2023

Seconding @briansmith 's comment from soon 5 years ago, is there anything blocking stabilization of the basic and uncontroversial methods like rotate_left|right, count_zeroes|ones, etc.?

It doesn't seem to me that requiring the nightly compiler is justified for trivial 1-line methods that have been around for many years now.

Given the lack of discussion here, can one directly send a PR to stabilize some methods?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests