Fix reliance on the sign of char in fnv32 and fnv64

Summary:
Clearly someone noticed this was an issue before and fixed the other 2 overloads, but never fixed these :(

Specifically, ARM has unsigned chars, so this would be producing the wrong values there.

Reviewed By: yfeldblum, mzlee

Differential Revision: D5122992

fbshipit-source-id: eb71dea82c187c09c2c4496f1f7c99bd01db6dd0

diff --git a/folly/Hash.h b/folly/Hash.h
index 1df1ff67fc04eea30d50ab8deffb30ce43016b3c..77ba5878f17e6502c0c89a0e2fbe42f8c4dc757f 100644 (file)
--- a/folly/Hash.h
+++ b/folly/Hash.h
@@ -215,8 +215,10 @@ inline uint32_t jenkins_rev_unmix32(uint32_t key) {
 const uint32_t FNV_32_HASH_START = 2166136261UL;
 const uint64_t FNV_64_HASH_START = 14695981039346656037ULL;
 
 const uint32_t FNV_32_HASH_START = 2166136261UL;
 const uint64_t FNV_64_HASH_START = 14695981039346656037ULL;
 

-inline uint32_t fnv32(const char* s,
-                      uint32_t hash = FNV_32_HASH_START) {
+inline uint32_t fnv32(const char* buf, uint32_t hash = FNV_32_HASH_START) {
+  // forcing signed char, since other platforms can use unsigned
+  const signed char* s = reinterpret_cast<const signed char*>(buf);
+

   for (; *s; ++s) {
     hash += (hash << 1) + (hash << 4) + (hash << 7) +
             (hash << 8) + (hash << 24);
   for (; *s; ++s) {
     hash += (hash << 1) + (hash << 4) + (hash << 7) +
             (hash << 8) + (hash << 24);


@@ -245,8 +247,10 @@ inline uint32_t fnv32(const std::string& str,
   return fnv32_buf(str.data(), str.size(), hash);
 }
 
   return fnv32_buf(str.data(), str.size(), hash);
 }
 

-inline uint64_t fnv64(const char* s,
-                      uint64_t hash = FNV_64_HASH_START) {
+inline uint64_t fnv64(const char* buf, uint64_t hash = FNV_64_HASH_START) {
+  // forcing signed char, since other platforms can use unsigned
+  const signed char* s = reinterpret_cast<const signed char*>(buf);
+

   for (; *s; ++s) {
     hash += (hash << 1) + (hash << 4) + (hash << 5) + (hash << 7) +
       (hash << 8) + (hash << 40);
   for (; *s; ++s) {
     hash += (hash << 1) + (hash << 4) + (hash << 5) + (hash << 7) +
       (hash << 8) + (hash << 40);

diff --git a/folly/test/HashTest.cpp b/folly/test/HashTest.cpp
index 7e6bcdf0573944f35748903a2f7cfa479c0c1cd3..01cbf2e39e96f917731706f80b1b212bf5a8b1ea 100644 (file)
--- a/folly/test/HashTest.cpp
+++ b/folly/test/HashTest.cpp
@@ -38,6 +38,12 @@ TEST(Hash, Fnv32) {
   const uint32_t s3_res = 2166136261UL;
   EXPECT_EQ(fnv32(s3), s3_res);
   EXPECT_EQ(fnv32(s3), fnv32_buf(s3, strlen(s3)));
   const uint32_t s3_res = 2166136261UL;
   EXPECT_EQ(fnv32(s3), s3_res);
   EXPECT_EQ(fnv32(s3), fnv32_buf(s3, strlen(s3)));

+
+  const uint8_t s4_data[] = {0xFF, 0xFF, 0xFF, 0x00};
+  const char* s4 = reinterpret_cast<const char*>(s4_data);
+  const uint32_t s4_res = 2420936562UL;
+  EXPECT_EQ(fnv32(s4), s4_res);
+  EXPECT_EQ(fnv32(s4), fnv32_buf(s4, strlen(s4)));

 }
 
 TEST(Hash, Fnv64) {
 }
 
 TEST(Hash, Fnv64) {


@@ -56,6 +62,12 @@ TEST(Hash, Fnv64) {
   EXPECT_EQ(fnv64(s3), s3_res);
   EXPECT_EQ(fnv64(s3), fnv64_buf(s3, strlen(s3)));
 
   EXPECT_EQ(fnv64(s3), s3_res);
   EXPECT_EQ(fnv64(s3), fnv64_buf(s3, strlen(s3)));
 

+  const uint8_t s4_data[] = {0xFF, 0xFF, 0xFF, 0x00};
+  const char* s4 = reinterpret_cast<const char*>(s4_data);
+  const uint64_t s4_res = 2787597222566293202ULL;
+  EXPECT_EQ(fnv64(s4), s4_res);
+  EXPECT_EQ(fnv64(s4), fnv64_buf(s4, strlen(s4)));
+

   // note: Use fnv64_buf to make a single hash value from multiple
   // fields/datatypes.
   const char* t4_a = "E Pluribus";
   // note: Use fnv64_buf to make a single hash value from multiple
   // fields/datatypes.
   const char* t4_a = "E Pluribus";