-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
extending Saturating to include other arithmetic operations #15926
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
Conversation
|
I don't like adding these into a single Also, I think mul/div weren't provided because nobody needed that functionality. |
|
@kballard I agree that putting them all in one trait is silly, but I think it's worthwhile to provide saturating versions of all ops, if any. I'm willing to break them into there own individual traits StaturatingAdd/Sub/Mul/Div, if that's what you're looking for. |
|
What is the motivating reason for extending this to mul/div? It's already pretty easy to get the same functionality with the "checked" operations if anyone really needs it. I gather you have no actual place where you want to use this, right? It's usually not a great idea to introduce APIs with no known clients. |
|
I temporarily was a client for it while working on the iterator stuff (I was hacking in saturating_mul by using a.saturating_add(a)), but it became more natural to use Checked directly in that case. You're right that it's not hard for a user to write their own versions of all the saturating operators given Checked versions, so this isn't a huge value added. I was under the impression that we simply were interested in offering the three major possible solutions to handling arithmetic overflow: wrap, fail, and saturate. At very least, if you don't want to support saturating mul/div, surely your reasoning implies that Saturate should be broken into two Traits? |
If you have a use-case for For the record, I'm not sure |
|
Alright, I'm willing to toss this on the basis of saturating being uninteresting for general usage. No real skin off my teeth. Code's here if anyone ever changes their mind, not that it was a big deal to write. As an aside saturating_div makes sense to me because you can overflow (with the div by -1 you note). It's very much so a corner-case, and damn weird for it to be done (and considered), but it's there. The whole thing's also written for "Num" so there's no reason why someone couldn't write their own exotic Num type where it's more relevant, right? |
- Make the diagnostic message actually desribe the problem - Always give the suggestion, by using `snippet_with_context` - Make the suggestion verbose, because we sometimes lint multiline exprs like `match`es changelog: [`manual_option_as_slice`]: improve diagnostics
I see no reason not to support mul/div here. Div is perhaps a bit over-engineered since there's no plausible mechanism to get to the min_value branch using our integer types (which are the only ones we support right now). Still, a theoretical Num type could reach there.
I'm a bit irked that I basically regress the div-by-zero checking provided by CheckedDiv, but I see no way to reasonably handle it.
I can't seem to find any existing tests for Saturating. I'd be more than happy to write tests if someone could point me to the right place.