Skip to content

Conversation

@Gankra
Copy link
Contributor

@Gankra Gankra commented Jul 23, 2014

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.

@lilyball
Copy link
Contributor

I don't like adding these into a single Saturating trait. That prevents anyone from using saturating math on a type that implements CheckedAdd/CheckedDiv without CheckedMul/CheckedDiv.

Also, I think mul/div weren't provided because nobody needed that functionality. Saturating exists primarily because iterator size hints really want to use it, but offhand I'm not sure what else, if anything, uses it.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 23, 2014

@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.

@lilyball
Copy link
Contributor

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.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 23, 2014

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?

@lilyball
Copy link
Contributor

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?

Saturating literally only exists to make Iterator.size_hint() easier to implement. That's why it's a single trait, and that's why it takes the argument by-value instead of by-reference. It was designed purely for convenience, and is not really expected to be used anywhere else.

If you have a use-case for Saturating outside of size_hint(), then it might make sense to promote it from "convenience trait" to a fully-fledged numeric trait. But without a real use-case I think it's unnecessary work.

For the record, I'm not sure saturating_div even makes sense to provide. What is it going to saturate? Our "checked" traits are only implemented on integral types. The only two cases I can think of where you even need checked division are when either a) you might be dividing MIN by -1, or b) you might be dividing by 0. And neither case really makes much sense in the context of a saturating operation.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 23, 2014

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?

@Gankra Gankra closed this Jul 23, 2014
@Gankra Gankra deleted the saturating branch August 18, 2014 19:10
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 31, 2025
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants