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

smhasher: Set UNALIGNED_SAFE for 64-bit intel too. #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nico
Copy link

@nico nico commented Mar 4, 2020

This is better for perf, and also happens to suppress a new clang warning:

PMurHash.c(209,12): error: cast to smaller integer type 'long' from
'const unsigned char *' is a Microsoft extension [-Werror,-Wmicrosoft-cast]
int i = -(long)ptr & 3;
^~~~~~~~~

64-bit Windows is the only platform I know where long is smaller than
a pointer, and this code is in the !UNALIGNED_SAFE branch. So this patch
addresses the warning.

(Alternatively, we could insert a cast to (uintptr_t) before casting to
long, but the code seems to try to work before C99.)

https://crbug.com/1054220

This is better for perf, and also happens to suppress a new clang warning:

  PMurHash.c(209,12): error: cast to smaller integer type 'long' from
    'const unsigned char *' is a Microsoft extension [-Werror,-Wmicrosoft-cast]
  int i = -(long)ptr & 3;
  ^~~~~~~~~

64-bit Windows is the only platform I know where long is smaller than
a pointer, and this code is in the !UNALIGNED_SAFE branch. So this patch
addresses the warning.

(Alternatively, we could insert a cast to (uintptr_t) before casting to
long, but the code seems to try to work before C99.)

https://crbug.com/1054220
@nico
Copy link
Author

nico commented Mar 5, 2020

I forgot about amd64; we'll have to patch the warning in the unaligned branch for that. But this seems like a good change regardless.

pull bot pushed a commit to FreddyZeng/chromium that referenced this pull request Mar 15, 2020
The code causes a new compile warning, and it's unused. So stop
building it.

(See also aappleby/smhasher#81 failing
to get traction.)

Bug: 1054220,1058381
Change-Id: I68758e94cd2728df70e78c60e9be0bb4006a847a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103719
Auto-Submit: Nico Weber <[email protected]>
Commit-Queue: Daniel Cheng <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Cr-Commit-Position: refs/heads/master@{#750421}
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.

1 participant