Skip to content

Remove redundant bit masking #5654

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.

Sign up for GitHub

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

Jump to bottom
Closed
findepi wants to merge 1 commit into from
Closed

Remove redundant bit masking #5654

findepi wants to merge 1 commit into from

Conversation

@findepi
Copy link
Contributor

@findepi findepi commented Jul 20, 2021

No description provided.

@google-cla google-cla bot added the cla: yes label Jul 20, 2021
@eamonnmcmanus eamonnmcmanus self-assigned this Jul 20, 2021
eamonnmcmanus
eamonnmcmanus requested changes Jul 20, 2021
View reviewed changes
Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

I agree with the idea behind this PR. The existing code has a rather weird way of implementing the UTF-8 principle where the number of high 1 bits in the first byte indicates how many bytes there are. Your rewrite makes this clearer, since the masks are 0x3, 0x7, 0xF for 2, 3, or 4 bytes respectively.

@findepi findepi force-pushed the findepi/remove-redundant-bit-masking-467c13 branch from db50046 to d0bb0fe Compare July 21, 2021 11:39
@findepi
Copy link
Contributor Author

findepi commented Jul 21, 2021

@eamonnmcmanus added assert (can replace with a comment if you prefer so).
Also added test cases which exercise edge case values.

@findepi findepi force-pushed the findepi/remove-redundant-bit-masking-467c13 branch from d0bb0fe to 8685091 Compare July 21, 2021 11:47
eamonnmcmanus
eamonnmcmanus reviewed Jul 21, 2021
View reviewed changes
@findepi findepi force-pushed the findepi/remove-redundant-bit-masking-467c13 branch from 8685091 to b187f9d Compare July 21, 2021 20:22
@findepi
Copy link
Contributor Author

findepi commented Jul 21, 2021

@eamonnmcmanus changed to comments
also, added a clarifying TODO comment in the test case, where i had to use an incorrect expected value.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Jul 21, 2021

Thanks, this looks fine. Because of the way we sync between our internal repo and GitHub, we'll be closing this PR and sending out an equivalent one, duly attributed to you.

@findepi
Copy link
Contributor Author

findepi commented Jul 22, 2021

Thanks for review

@nick-someone nick-someone added P3 no SLO package=hash type=performance Related to performance labels Jul 26, 2021
@copybara-service copybara-service bot mentioned this pull request Jul 29, 2021
Merged
copybara-service bot pushed a commit that referenced this pull request Jul 29, 2021
@eamonnmcmanus
Remove redundant bit masking.
4ab5c8c 
Also add some more test cases for `hashString`.

Closes #5654.

RELNOTES=n/a
PiperOrigin-RevId: 386122757
@findepi findepi deleted the findepi/remove-redundant-bit-masking-467c13 branch July 30, 2021 07:17
@findepi
Copy link
Contributor Author

findepi commented Jul 30, 2021

@eamonnmcmanus thanks for merging

@findepi findepi mentioned this pull request Aug 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eamonnmcmanus eamonnmcmanus eamonnmcmanus requested changes

Assignees

@eamonnmcmanus eamonnmcmanus

Labels

cla: yes P3 no SLO package=hash type=performance Related to performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@findepi @eamonnmcmanus @nick-someone