[PATCH] D122974: prevent ConstString from calling djbHash() more than once

[Greg Clayton via Phabricator via llvm-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/source/Utility/ConstString.cpp:177-183
   uint8_t hash(const llvm::StringRef &s) const {
-    uint32_t h = llvm::djbHash(s);
+    return hash(StringPool::hash(s));
+  }
+
+  uint8_t hash(uint32_t h) const {
     return ((h >> 24) ^ (h >> 16) ^ (h >> 8) ^ h) & 0xff;
   }
----------------
Can we rename these functions to something other than "hash"? It is a bit confusing to have a function called "hash" that returns a uint8_t when what this is really doing is getting use the string pool index. Maybe "GetPoolIndex"? I could see this function accidentally being used to hash a string instead of using StringPool::hash()


================
Comment at: llvm/include/llvm/ADT/StringMap.h:69
+  /// Overload that explicitly takes precomputed hash(Key).
+  unsigned LookupBucketFor(StringRef Key, unsigned FullHashValue);
 
----------------
Use "uint32_t" instead of unsigned to clarify the exact width of the integer. Or create a "HashType" typedef and then use that.


================
Comment at: llvm/include/llvm/ADT/StringMap.h:106
 
+  static unsigned hash(StringRef Key) { return llvm::djbHash(Key, 0); }
+
----------------



================
Comment at: llvm/lib/Support/StringMap.cpp:83
+unsigned StringMapImpl::LookupBucketFor(StringRef Name,
+                                        unsigned FullHashValue) {
+#ifdef EXPENSIVE_CHECKS
----------------



================
Comment at: llvm/lib/Support/StringMap.cpp:141
 /// This does not modify the map.
-int StringMapImpl::FindKey(StringRef Key) const {
+int StringMapImpl::FindKey(StringRef Key, unsigned FullHashValue) const {
   if (NumBuckets == 0)
----------------



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122974/new/

https://reviews.llvm.org/D122974