-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 missing Wrapping methods, use doc_comment! #49393
Conversation
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
r? @SimonSapin |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The actual error is doc-tests in
|
Will fix that later today. Also was thinking of revamping the |
Sorry for giving you a bad commit; I sent a PR for the fix this morning: clarfonthey#2 |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Sorry, @clarcharr; apparently I shouldn't try to do a quick fix before leaving for work. Hopefully this time I got it right (as I waited for the annoying stage2 build for rustdoc, and the doctests passed locally). |
@scottmcm No worries; thanks for the changes. :) |
Ping from triage @SimonSapin! This PR needs your review. |
|
As far as the docs go, I was planning on moving these docs to use As far as the boolean methods: I personally agree, although I'd use #44724 as a precedent for these being there. There is absolutely no use case for changing the endianness of a single byte, but having the methods exist on all integer types makes creating your own traits a lot easier, as you won't have to handle For the usefulness of |
I think these are all consistency arguments, making sure that replacing
And like how one of the arguments for |
I hear the argument that |
Perhaps wrapping to one might be a better solution? |
Well, the next power of two after 200 is definitely 256, and 256 = 0 mod 256, so I do think that wrapping to 0 is the correct output. And I think "doesn't give you a power of two" is no more surprising or a footgun than
|
I feel like we’re all making suppositions on a theoretical basis here, and maybe we’d better off not add this method until someone comes along who actually wants to use it. Then we can look at the use case and see what behavior is most useful and least of a footgun. |
☔ The latest upstream changes (presumably #49939) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage! What's the status on this? |
Thanks for the PR @clarcharr ! Since we haven't heard from you in a few weeks, I'm going to go ahead and close this PR for now to keep the queue tidy. If you have time to address the feedback, we'd love for you to reopen this PR! |
I finally got around to doing what I wanted for this PR, so, I'm going to open it up again. This adds all of the missing methods from the tracking issue, and uses |
Err, I can't actually open it, so, I'll let @shepmaster or someone else do it. (If it makes more sense to open a new PR, I'll do that.) |
Well, that's annoying. We should probably warn people about that in our triage message. Sorry @clarcharr ! |
Add missing Wrapping methods, use doc_comment! Re-opened version of #49393 . Finishing touches for #32463. Note that this adds `Shl` and `Shr` implementations for `Wrapping<i128>` and `Wrapping<u128>`, which were previously missed. This is technically insta-stable, but I don't know why this would be a problem.
Part of #32463.