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

[3.2] avoid redefining _mm_crc32_u64 which can lead to compile errors #242

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

spoonincode
Copy link
Member

Defining our own _mm_crc32_u64 implementation can clash with some compilers. Not too surprising since that's a reserved name in global scope. In particular, AppleClang will give an error:

error: always_inline function '_mm_crc32_u64' requires target feature 'sse4.2', but would be inlined into function 'CityHashCrc256Long' that is compiled without support for 'sse4.2'

Rename our impl to just mm_crc32_u64 and redirect to either mm_crc32_u64 or _mm_crc32_u64 via a new macro.

#else
uint64_t _mm_crc32_u64(uint64_t a, uint64_t b );
uint64_t mm_crc32_u64(uint64_t a, uint64_t b );
#define MM_CRC32_U64I mm_crc32_u64
Copy link
Member

Choose a reason for hiding this comment

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

Should we use FC_MM_CRC32_U64I instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to keep the same length to avoid touching a bunch of whitespace over in CHUNK() 😅

Since this define is local to a .cpp it doesn't seem like there is much risk of a clash? Possibly mm_crc32_u64() should be moved in to fc's namespace though?

Copy link
Member Author

@spoonincode spoonincode Sep 27, 2022

Choose a reason for hiding this comment

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

actually if _mm_crc32_u64 is just placed in to a namespace I think that's safe. maybe that's the better choice 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice this was only in this one source file. I think it is fine as is.

#else
uint64_t _mm_crc32_u64(uint64_t a, uint64_t b );
uint64_t mm_crc32_u64(uint64_t a, uint64_t b );
#define MM_CRC32_U64I mm_crc32_u64
Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice this was only in this one source file. I think it is fine as is.

Base automatically changed from desubmod_fc to main September 27, 2022 23:20
@spoonincode spoonincode merged commit b99e842 into main Sep 27, 2022
@spoonincode spoonincode deleted the mm_crc32_u64 branch September 27, 2022 23:21
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.

None yet

2 participants