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

wrapping_$op() and saturating_$op() should be #[must_use] #59787

Closed
tormol opened this issue Apr 7, 2019 · 5 comments
Closed

wrapping_$op() and saturating_$op() should be #[must_use] #59787

tormol opened this issue Apr 7, 2019 · 5 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tormol
Copy link
Contributor

tormol commented Apr 7, 2019

I just had a bug due to a wrapping_add() having no effect because the return value was ignored:

let mut foo = foo();
foo.x.wrapping_add(bar); // should be foo.x = foo.x.wrapping_add(bar);
foo.y = baz; // so no warning about foo not needing to be mutable

It's an easy mistake to make when rewriting foo.x += bar to handle overflow.

EDIT: (I didn't do enough searching before opening this issue)

On one hand this is a subset of #48926, and covers a lot of methods so RFC #2450 would be better. On the other hand this issue is a v2 of #50124 for the parts of it not covered by #50149.

Meta

rustc 1.35.0-nightly (f8673e0ad 2019-04-03)
binary: rustc
commit-hash: f8673e0ad85e98997faa76fa7edc99c5825f46ee
commit-date: 2019-04-03
host: x86_64-unknown-linux-gnu
release: 1.35.0-nightly
LLVM version: 8.0
@tesuji
Copy link
Contributor

tesuji commented Apr 8, 2019

What do you mean? If you comment the foo.y line, you would get the unused warning:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=15090d92e645a5cdcd2af9071c92e3d9

@alex
Copy link
Member

alex commented Apr 8, 2019

I would expect, in either version, a warning on the wrapping_add line about an unused result.

@varkor
Copy link
Member

varkor commented Apr 9, 2019

I agree that this could have been included as part of #50124, so I don't think it'd be controversial to open a pull request adding #[must_use] to the wrapping_* variants.

@memoryruins
Copy link
Contributor

+1, I've seen this bite a few users in chats. searching logs, one said:

I'm using wrapping_add in yet another obligatory chip8 emulator and didn't store the result in a register, took me a few hours to find. thing is, I'm using that because of the panic for overflows, ironically exactly that is what caused me to write this bug

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 21, 2019
@Mark-Simulacrum
Copy link
Member

AFAICT, this has since been done. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

7 participants