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

Remove trait inheritance from CheckedAdd/Sub/Mul/Div #12275

Closed

Conversation

lilyball
Copy link
Contributor

There's no need for these traits to inherit from Add/Sub/Mul/Div.

There's no need for these traits to inherit from Add/Sub/Mul/Div.
@thestinger
Copy link
Contributor

If you can perform operations returning Option, then you can also perform ones not returning Option. I don't see a reason to remove the inheritance here.

@lilyball
Copy link
Contributor Author

@thestinger This was motivated by my desire to impl CheckedAdd for char, though I'm unlikely to actually do that. I still thought it was worth submitting because there's no reason for CheckedAdd to require Add. Your statement that you can always have a non-Option variant only seems to work for types that wrap. Non-wrapping types can't necessarily have an unchecked variant, same with types that have invalid holes within their range (as char does, with the 0xD800-0xDFFF block).

@thestinger
Copy link
Contributor

They can always fail/abort for the None case. This is what the non-Checked variant of division already does.

@lilyball
Copy link
Contributor Author

That's pretty terrible. I would expect a + b to never fail. Division is more acceptable because everyone knows you can't divide by 0. And that's still not a reason to make CheckedFoo inherit from Foo.

@thestinger
Copy link
Contributor

It inherits from it because it's a superset of the functionality. Trait inheritance makes writing generic code more convenient because you don't need to specify as many bounds. Without at least one example of a type where it's sensible to implement CheckedAdd but not Add, I definitely don't think the inheritance relationship should be removed.

@lilyball
Copy link
Contributor Author

The only person who has chimed in so far is @thestinger, who is against it. I'm going to assume nobody else particularly cares, and as such I will close this until such time as I find a use for a CheckedAdd that doesn't have Add.

@lilyball lilyball closed this Feb 17, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
…, r=jonas-schievink

fix: Don't generate documentation in `generate_setter`

Followup to rust-lang/rust-analyzer#12274
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 26, 2024
…anishearth

[`incompatible_msrv`]: allow expressions that come from desugaring

Fixes rust-lang#12273

changelog: [`incompatible_msrv`]: don't lint on the `IntoFuture::into_future` call desugared by `.await`
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.

None yet

2 participants