Cyan4973 / xxHash

Extremely fast non-cryptographic hash algorithm
http://www.xxhash.com/
Other
9.04k stars 773 forks source link

Compile issue with gcc 11 -std=c++23 #791

Closed eseiler closed 1 year ago

eseiler commented 1 year ago

Compiling with gcc-11 -std=c++23:

xxhash.h:2052:34: error: ‘unreachable’ is not a member of ‘std’
 2052 | #  define XXH_UNREACHABLE() std::unreachable()

782 introduced

https://github.com/Cyan4973/xxHash/blob/6dba9abfaae75a4b70a5c53d7e5eb3dd554973d1/xxhash.h#L2049-L2052

The problem with __cplusplus is that it does not necessarily mean that all features are available. It's usually more robust to use the feature testing macros (__cpp_lib_unreachable). In this case, it would mean using

#elif defined(__cpp_lib_unreachable) && (__cpp_lib_unreachable >= 202202L)

See also https://godbolt.org/z/5Ys77oM9j

I am not sure if this also applies to the C version. GCC defines __STDC_VERSION_STDDEF_H__, but I haven't found a reference for this.

Cyan4973 commented 1 year ago

cc @goldsteinn

goldsteinn commented 1 year ago

Compiling with gcc-11 -std=c++23:

xxhash.h:2052:34: error: ‘unreachable’ is not a member of ‘std’
 2052 | #  define XXH_UNREACHABLE() std::unreachable()

782 introduced

https://github.com/Cyan4973/xxHash/blob/6dba9abfaae75a4b70a5c53d7e5eb3dd554973d1/xxhash.h#L2049-L2052

The problem with __cplusplus is that it does not necessarily mean that all features are available. It's usually more robust to use the feature testing macros (__cpp_lib_unreachable). In this case, it would mean using

#elif defined(__cpp_lib_unreachable) && (__cpp_lib_unreachable >= 202202L)

See also https://godbolt.org/z/5Ys77oM9j

Thank you, will update the CPP version to this.

I am not sure if this also applies to the C version. GCC defines __STDC_VERSION_STDDEF_H__, but I haven't found a reference for this.

By not sure if this applies to C, do you mean you're not sure if C can have the same issue if not-implement features, or you're not sure what the feature test define for something is?

My guess is if there is concern about it, we don't lose much dropping the C version. A compiler that doesn't implement some builtin for unreachable/assume is also likely to not use it an analysis even if it recognizes to stay conformant. Based on HEDLEY most of the common optimizing compilers are good with assume or __builtin_unreachable. @Cyan4973 what do you think?

goldsteinn commented 1 year ago

Fix is here: https://github.com/Cyan4973/xxHash/pull/792

Dropped the c23 version there as well to avoid more issues, but can obviously drop that.

eseiler commented 1 year ago

By not sure if this applies to C, do you mean you're not sure if C can have the same issue if not-implement features, or you're not sure what the feature test define for something is?

Both. I'm not sure whether C has the same issue regarding __STDC_VERSION__, and whether there is a feature test macro for it. However, as pointed out here, unreachable was defined as a macro, so we can just check whether it's defined.

goldsteinn commented 1 year ago

@Cyan4973 should probably mark this resolved.