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

fix(strutil.h): ensure proper constexpr of string hashing #3901

Merged
merged 1 commit into from
Jul 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 64 additions & 8 deletions src/include/OpenImageIO/detail/farmhash.h
Original file line number Diff line number Diff line change
Expand Up @@ -239,21 +239,77 @@ STATIC_INLINE uint32_t Fetch32(const char *p) {

#else

template <typename T>
STATIC_INLINE T FetchType(const char *p) {
T result = 0;
for (size_t i = 0; i < sizeof(T); i++)
reinterpret_cast<char *>(&result)[i] = p[i];
return result;
#if defined(__cpp_lib_bit_cast) && __cpp_lib_bit_cast >= 201806L
// C++20 compiler with constexpr support std::bitcast
STATIC_INLINE uint64_t Fetch64(const char *p) {
struct BlockOfBytes {
char bytes[8];
};
BlockOfBytes bob{p[0],p[1],p[2],p[3],p[4],p[5],p[6],p[7]};
return std::bit_cast<uint64_t>(bob);
}
STATIC_INLINE uint32_t Fetch32(const char *p) {
struct BlockOfBytes {
char bytes[4];
};
BlockOfBytes bob{p[0],p[1],p[2],p[3]};
return std::bit_cast<uint32_t>(bob);
}

#else

// constexpr supported for C++14,17 versions that manually load bytes and
// bitshift into proper order for the unsigned integer.
// NOTE: bigendian untested
STATIC_INLINE uint64_t Fetch64(const char *p) {
return FetchType<uint64_t>(p);
// Favor letting uint8_t construction to handle
// signed to unsigned conversion vs. uint64_t(p[0]&0xff)
uint8_t b0 = p[0];
uint8_t b1 = p[1];
uint8_t b2 = p[2];
uint8_t b3 = p[3];
uint8_t b4 = p[4];
uint8_t b5 = p[5];
uint8_t b6 = p[6];
uint8_t b7 = p[7];
return littleendian() ?
(uint64_t(b0)) | // LSB
(uint64_t(b1) << 8) |
(uint64_t(b2) << 16) |
(uint64_t(b3) << 24) |
(uint64_t(b4) << 32) |
(uint64_t(b5) << 40) |
(uint64_t(b6) << 48) |
(uint64_t(b7) << 56) // MSB
: // Big Endian byte order
(uint64_t(b0) << 56) | // MSB
(uint64_t(b1) << 48) |
(uint64_t(b2) << 40) |
(uint64_t(b3) << 32) |
(uint64_t(b4) << 24) |
(uint64_t(b5) << 16) |
(uint64_t(b6) << 8) |
(uint64_t(b7)) ; // LSB
}

STATIC_INLINE uint32_t Fetch32(const char *p) {
return FetchType<uint32_t>(p);
uint8_t b0 = p[0];
uint8_t b1 = p[1];
uint8_t b2 = p[2];
uint8_t b3 = p[3];
return littleendian() ?
(uint32_t(b0)) | // LSB
(uint32_t(b1) << 8) |
(uint32_t(b2) << 16) |
(uint32_t(b3) << 24) // MSB
: // Big Endian byte order
(uint32_t(b0) << 24) | // MSB
(uint32_t(b1) << 16) |
(uint32_t(b2) << 8) |
(uint32_t(b3)); // LSB
}
#endif


// devices don't seem to have bswap_64() or bswap_32()
template<typename T>
Expand Down
19 changes: 16 additions & 3 deletions src/include/OpenImageIO/string_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,8 @@ class basic_string_view {
: m_chars(chars), m_len(len) { }

/// Construct from char*, use strlen to determine length.
OIIO_CONSTEXPR17 basic_string_view(const CharT* chars) noexcept
: m_chars(chars), m_len(chars ? Traits::length(chars) : 0) { }
// N.B. char_traits::length() is constexpr starting with C++17.
constexpr basic_string_view(const CharT* chars) noexcept
: m_chars(chars), m_len(chars ? cestrlen(chars) : 0) { }

/// Construct from std::string. Remember that a string_view doesn't have
/// its own copy of the characters, so don't use the `string_view` after
Expand Down Expand Up @@ -431,6 +430,20 @@ class basic_string_view {
return last;
}

// Guaranteed constexpr length of a C string
static constexpr size_t cestrlen(const charT* chars) {
#if OIIO_CPLUSPLUS_VERSION >= 17
return Traits::length(chars);
#else
if (chars == nullptr)
return 0;
size_t len = 0;
while (chars[len] != 0)
len++;
return len;
#endif
}

class traits_eq {
public:
constexpr traits_eq (CharT ch) noexcept : ch(ch) {}
Expand Down
3 changes: 3 additions & 0 deletions src/libutil/strutil_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,9 @@ test_hash()
OIIO_CHECK_EQUAL(strhash(std::string("foo")), 6150913649986995171);
OIIO_CHECK_EQUAL(strhash(string_view("foo")), 6150913649986995171);
OIIO_CHECK_EQUAL(strhash(""), 0); // empty string hashes to 0
// Check longer hash and ensure that it's really constexpr
constexpr size_t hash = Strutil::strhash("much longer string");
OIIO_CHECK_EQUAL(hash, 16257490369375554819ULL);
}


Expand Down
Loading