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

Reject '2' as a binary digit in internals of b: number formatting #46356

Merged
merged 1 commit into from Nov 29, 2017

Conversation

Projects
None yet
5 participants
@daboross
Contributor

daboross commented Nov 29, 2017

The radix! macro generates an implementation of the private trait GenericRadix, and the code replaced changes Binary's implementation to no longer accept '2' as a valid digit to print.

Granted, this code is literally only ever called from another method in this private trait, and that method has logic to never hand a '2' to the printing function. Even given this, the code's there, I thought it would be best to fix this for clarity of anyone reading it.

Reject '2' as a binary digit in internals of 'b' formatting
I don't believe the previous code `0 ... 2` would run into any real problems, but it seems confusing to read, given that '2' is never a valid binary digit.

As far as I can tell this code is only ever called from within another private method in the trait which has logic to never hand it '2' anyways. I thought we could change this for clarity anyways.
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 29, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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. Due to 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.

Collaborator

rust-highfive commented Nov 29, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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. Due to 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.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Nov 29, 2017

Member

@bors r+ rollup

Member

sfackler commented Nov 29, 2017

@bors r+ rollup

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 29, 2017

Contributor

📌 Commit 2c98378 has been approved by sfackler

Contributor

bors commented Nov 29, 2017

📌 Commit 2c98378 has been approved by sfackler

kennytm added a commit to kennytm/rust that referenced this pull request Nov 29, 2017

Rollup merge of rust-lang#46356 - daboross:patch-2, r=sfackler
Reject '2' as a binary digit in internals of b: number formatting

The `radix!` macro generates an implementation of the private trait `GenericRadix`, and the code replaced changes Binary's implementation to no longer accept '2' as a valid digit to print.

Granted, this code is literally only ever called from another method in this private trait, and that method has logic to never hand a '2' to the printing function. Even given this, the code's there, I thought it would be best to fix this for clarity of anyone reading it.

bors added a commit that referenced this pull request Nov 29, 2017

Auto merge of #46362 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45969, #46077, #46219, #46287, #46293, #46322, #46323, #46330, #46354, #46356
- Failed merges:

@bors bors merged commit 2c98378 into rust-lang:master Nov 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daboross daboross deleted the daboross:patch-2 branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment