-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add is_multiple_of
#404
Comments
Given that we have https://doc.rust-lang.org/std/primitive.u32.html#method.next_multiple_of, having
|
To me one of the upsides of the function as currently proposed is that it just never panics. While the next multiple of zero does not make sense, 0 is absolutely a multiple of 0. |
I would love to see this. @folkertdev Do you actually need this for signed types, or would your use cases be covered if this just supported unsigned types (like |
@joshtriplett I believe I've personally only ever wanted/used this function for unsigned types, but also I think the behavior of So at the moment I see no reason not to just include signed types in one fell swoop, but if that is easier/faster then they could be split up of course. |
Mostly asking for bikeshed avoidance reasons: If we know that unsigned is the primary use case, when we go to discuss this if we have any disagreements over the signed versions we can focus on consensus on the unsigned versions. |
That's not necessarily better, if it makes the post-condition less clear. Can you show more about the situations in which it's used, and what people are doing based on the answer this gives? |
To me, the postcondition is very clear: the function answers whether for its input lhs and rhs there exists an integer I'll agree that it is weird, in that with the standard So I guess the question is whether there are cases where the current Sometimes, a check for whether the rhs is zero is already required for correct logic. In those cases, the change in panicking behavior has no impact on the result of the program // If src type size is an integer multiple of the destination type size then
// the caller will have calculated a `dst_cap` that is an integer multiple of
// `src_cap` without remainder.
if const {
let src_sz = mem::size_of::<SRC>();
let dest_sz = mem::size_of::<DEST>();
// would become dest_sz != 0 && src_sz.is_multiple_of(dest_sz)
dest_sz != 0 && src_sz % dest_sz == 0
} {
return false;
} Other times, the panic is actually a bug: here i believe the behavior is correct when using for (i, ch) in s.chars().filter(|&ch| ch != '_').rev().enumerate() {
// would become: i > 0 && i.is_multiple_of(group_size)
if i > 0 && i % group_size == 0 {
chars.push('_');
}
chars.push(ch);
} In other cases I think the bug would be obvious even without the panic, or in any case just as obvious as any typo in the constant values. // would become: self.index.is_multiple_of(CHUNK_BITS)
if self.index % CHUNK_BITS == 0 {
break;
} const CHUNK_SIZE: usize = 192;
// Check the properties of `CHUNK_SIZE` and `UNROLL_INNER` that are required
// for correctness.
const _: () = assert!(CHUNK_SIZE < 256);
// would become: assert!(CHUNK_SIZE.is_multiple_of(UNROLL_INNER));
const _: () = assert!(CHUNK_SIZE % UNROLL_INNER == 0); in summary, I think the change in behavior is unlikely to come up, and even more unlikely to cause issues in practice, and actually quite likely to eliminate hidden bugs in current code. |
I actually tried to use As a semi-silly, semi-serious idea, you could have a |
For more prior art, Raku has the |
Option 1: We approve this for unsigned types only, and dodge the question. Odds are, most people won't actually care about the missing methods. Option 2: We implement this for signed types as well, with the semantic @folkertdev suggests. These seem like the two most reasonable options for us to decide between. I'm fine with either of these options, with a mild preference for the former. |
We discussed this in the @rust-lang/libs-api meeting and would like to add this only for unsigned integers for now. The signed variant can be deferred until there is a convincing use case. |
Proposal
Problem statement
Provide a convenient and panic-free method to check whether one numeric value is a multiple of another.
Motivating examples or use cases
Checking for "is x a multiple of y" is usually done with the expression
x % y == 0
. However, there are two subtle cases in which this expression does not work as intended:y == 0
, this operation will perform a division by zero.y == -1
andx == i{8, 16, 32, 64, 128}::MIN
, then the operation will overflow.These cases are unlikely to occur in practice, because determining whether a value is a multiple of 0 or -1 is kind of silly. Nonetheless, the potential panic because of division by zero or overflow is always there.
I'd also argue that
x % y == 0
isn't intuitive. It's a pattern we learn as programmers but is actually kind of hard to explain to newcomers.Though I've wanted this functionality before, it came up just now in a PR for miri. Miri's codebase is (rightfully!) averse to operations that can panic. However I know that because the right-hand side in my case is not 0 or -1, the panic is actually unreachable. Nonetheless, I have to pollute my code with unwraps.
Solution sketch
A version of this for all the
{i, u}*
typesThere might be some slight variations that produce better assembly. I think the name aligns well with
next_multiple_of
.In some real code:
Alternatives
Links and related work
What happens now?
This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
Possible responses
The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):
Second, if there's a concrete solution:
The text was updated successfully, but these errors were encountered: