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

Correct and clarify integer division rounding docs #26981

Merged
merged 2 commits into from Jul 13, 2015

Conversation

wthrowe
Copy link
Contributor

@wthrowe wthrowe commented Jul 12, 2015

This resolves #26845.

I'm not entirely satisfied with the placement of the rounding discussion in the docs for the Div and Rem traits, but I couldn't come up with anywhere better to put it. Suggestions are welcome.

I didn't add any discussion of rounding to the checked_div (or rem) or wrapping_div documentation because those seem to make it pretty clear that they do the same thing as Div.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Gankra
Copy link
Contributor

Gankra commented Jul 12, 2015

You can actually document trait implementations. example

The mock-impl of Div and Rem for all the primitives is here: https://doc.rust-lang.org/nightly/src/core/ops.rs.html#354-368

In principle you could split out the macro into two; one for integers and one for floats and it will show up on e.g. this page: https://doc.rust-lang.org/std/primitive.u8.html but forward_ref_binop might get in the way of doing this satisfactorily.

@alexcrichton
Copy link
Member

r? @gankro

@rust-highfive rust-highfive assigned Gankra and unassigned alexcrichton Jul 12, 2015
@Gankra
Copy link
Contributor

Gankra commented Jul 12, 2015

Another downside of the impl solution is that the details will be incredibly buried. That impl page is pretty crazy. @wthrowe thoughts?

@wthrowe
Copy link
Contributor Author

wthrowe commented Jul 13, 2015

I've tried moving the docs to the specific impls. I agree that both solutions have downsides, but I think I like the new version better. I expect most people who want this will be searching for it, so as long as it is on a reasonable page it's probably OK.

The forward_ref_binop stuff does mean that only the main impl gets documented, but I think that's alright. People can probably guess that they all do the same thing.

I wonder if making the documentation generator put things with actual documentation at the top would be a good idea. It would make stuff like this stand out more, but it would also separate related entries in the list if some have documentation and others don't (like the actual and ref implementations in my latest commit). I also have no idea how annoying it would be to code.

@Gankra
Copy link
Contributor

Gankra commented Jul 13, 2015

@bors r+

It probably wouldn't be too hard to teach rustdoc to frob the impl ordering based on whether the documentation is empty or not.

@bors
Copy link
Contributor

bors commented Jul 13, 2015

📌 Commit 7824956 has been approved by Gankro

@bors
Copy link
Contributor

bors commented Jul 13, 2015

⌛ Testing commit 7824956 with merge 5f552a5...

bors added a commit that referenced this pull request Jul 13, 2015
This resolves #26845.

I'm not entirely satisfied with the placement of the rounding discussion in the docs for the `Div` and `Rem` traits, but I couldn't come up with anywhere better to put it.  Suggestions are welcome.

I didn't add any discussion of rounding to the `checked_div` (or rem) or `wrapping_div` documentation because those seem to make it pretty clear that they do the same thing as `Div`.
@bors bors merged commit 7824956 into rust-lang:master Jul 13, 2015
@bors bors mentioned this pull request Jul 13, 2015
@wthrowe wthrowe deleted the div_docs branch July 15, 2015 13:06
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.

Integer division/remainder rounding behavior should be documented
5 participants