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

Improve computation of offset in EscapeUnicode #31253

Merged
merged 4 commits into from
Apr 20, 2016

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Jan 28, 2016

Unify the computation of offset and use leading_zeros instead of manually scanning the bits.
This PR removes some duplicated code and makes it a little simpler .
The computation of offset is also faster, but it is unlikely to have an impact on actual code.

(split from #31049)

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

// digit should be printed and (which is the same) avoids the
// (31 - 32) underflow
let msb = 31 - (c | 1).leading_zeros();
let msdigit = msb / 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is pretty confusing (I had to re-interpret it several times), so bear with me…

Should this variable be named nibbles/hex_digits (to-be-output) or something along the lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that could be misleading, because it is not the number of (hex)digits, but the index of the most significant one.
I hoped that the parallel with MSB would guide the reader and I thought that that presenting it as number of hex_digits would make them believe that an hex_digits = 0 would not be acceptable, while it is the correct offset for any c between 0 and 0xf.

The `offset` value was computed both in `next` and in `size_hint`;
computing it in a single place ensures consistency and makes it easier
to apply improvements. The value is now computed as soon as the
iterator is constructed. This means that the time to compute it is
spent immediately and cannot be avoided, but it also guarantees that
it is only spent once.
Instead of iteratively scanning the bits, use `leading_zeros`.
@ranma42 ranma42 force-pushed the improve-unicode-iter-offset branch from 283f3b8 to 79dfa25 Compare January 28, 2016 14:13
@ranma42
Copy link
Contributor Author

ranma42 commented Jan 28, 2016

I tried to improve the naming (offset -> hex_digit_idx, msdigit -> ms_hex_digit) and to add some comments.

@brson
Copy link
Contributor

brson commented Jan 29, 2016

I don't feel qualified to review this. Anybody know who is? @SimonSapin maybe?

@SimonSapin
Copy link
Contributor

I believe this code is equivalent to the one before, and it does look nicer. I haven’t benchmarked it to check if it’s faster.

@brson
Copy link
Contributor

brson commented Apr 20, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2016

📌 Commit 8984242 has been approved by brson

@bors
Copy link
Contributor

bors commented Apr 20, 2016

⌛ Testing commit 8984242 with merge 9cf6fba...

bors added a commit that referenced this pull request Apr 20, 2016
Improve computation of offset in `EscapeUnicode`

Unify the computation of `offset` and use `leading_zeros` instead of manually scanning the bits.
This PR removes some duplicated code and makes it a little simpler .
The computation of `offset` is also faster, but it is unlikely to have an impact on actual code.

(split from #31049)
@bors bors merged commit 8984242 into rust-lang:master Apr 20, 2016
@ranma42 ranma42 deleted the improve-unicode-iter-offset branch April 20, 2016 08:00
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.

6 participants