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

[metrohash] support more triplets by excluding 128 CRC source #16553

Merged
merged 4 commits into from
Mar 19, 2021
Merged

[metrohash] support more triplets by excluding 128 CRC source #16553

merged 4 commits into from
Mar 19, 2021

Conversation

luncliff
Copy link
Contributor

@luncliff luncliff commented Mar 5, 2021

What does your PR fix?

Make metrohash support more triplets, which are not available because of _mm_crc32_u64 intrinsic

  • Check VCPKG_TARGET_ARCHITECTURE to exclude source file that uses _mm_crc32_u64
  • Lower the standard to C++ 11 so the compilers with lower versions can build this port.
  • Minimize the CMake version constraint

Previous Work

Which triplets are supported/not supported? Have you updated the CI baseline?

metrohash related failures are removed from ci.baseline.txt. ARM & UWP triplets become available because there is no more constraint of _mm_crc32_u64

  • arm64-windows
  • arm64-uwp
  • x64-uwp

Does your PR follow the maintainer guide?

I thought we can create a patch for the header file, metrohash.h. It can add some macro checks like the following.

// ...
#ifndef METROHASH_METROHASH_H
#define METROHASH_METROHASH_H

#include "metrohash64.h" // <-- maybe we can replace "" to <> in the patch...
#include "metrohash128.h"
#if defined(_M_X64) || defined(__x86_64__)
// use SSE4.2 intrinsic _mm_crc32_u64
#include "metrohash128crc.h"
#endif
#endif // #ifndef METROHASH_METROHASH_H

I decided to remain simple for now but I'd like to hear about replacing vcpkg_replace_string with a patch like it.

luncliff added 3 commits March 5, 2021 23:47
* the change will allow x86 triplets
* update port SHA
* update ci.baseline.txt
* make both file use 'VCPKG_TARGET_TRIPLET' to make ease of comparison
@luncliff
Copy link
Contributor Author

luncliff commented Mar 5, 2021

Feels like my team is the only one who is using this port 🤔

@PhoebeHui PhoebeHui added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Mar 8, 2021
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

@luncliff, thanks for the PR!

@ras0219-msft, could you help further review?

@ras0219-msft
Copy link
Contributor

Thanks for the PR!

I've pushed a change that includes @Hoikas's suggestion. @luncliff could you confirm that this still covers all your use cases?

@luncliff
Copy link
Contributor Author

@ras0219-msft Thanks for the 1c8c750, I will check the port within days. :)

@luncliff
Copy link
Contributor Author

Works as expected for x86-android, x64-android, x86-ios, and x64-ios. Great!

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:discussion labels Mar 18, 2021
@ras0219-msft ras0219-msft merged commit 96403d0 into microsoft:master Mar 19, 2021
@luncliff luncliff deleted the port/metrohash branch March 20, 2021 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants