[PATCH] D22681: [esan] Add generic resizing hashtable

[Mike Aizatsky via llvm-commits]

aizatsky added inline comments.

================
Comment at: lib/esan/esan_hashtable.h:24
@@ +23,3 @@
+// KeyTy must have an operator== and an operator size_t() (for hashing).
+// Pointer types will be automatically de-referenced prior to either operator==
+// or a size_t hash cast.
----------------
bruening wrote:
> aizatsky wrote:
> > aizatsky wrote:
> > > Key pointer types...
> > I actually wonder if this is a functionality you want to build in rather then provide through type traits.
> > 
> > Like this:
> > 
> > template <typename KeyTy, typename DataTy, typename KeyTraits = DefaultTraits<KeyTy>>
> > 
> > And later something like:
> > 
> > KeyTraits::hash(key)
> > KeyTraits::eq(key1, key2).
> > 
> Could you elaborate on what the pros and cons of your proposal are?  I assume you mean that we define DefaultTraits to invoke operator== and operator size_t() and the point here is to let the user separate hashing from casting and key equality from some other equality -- though the existing constructor takes in an optional hash function for that purpose, and I'm not sure equality would need to be separated.
Pros:

- makes HashTable simpler
- moves type-based decision into a much-much simpler class 
- type-based decision can be made explicit. Consider:

  HashTable<int*, int> vs HashTable<int*, int, DerefPointers>

I would also suggest to move HashFunc to the trait as well. This would improve performance of the hashing.

The only const that I see is that you need to define a default trait struct. But that looks more like a pro to me :)

================
Comment at: lib/esan/esan_hashtable.h:28
@@ +27,3 @@
+// synchronization for payloads once hashtable functions return.  If
+// ExternalLock is set to true, the caller should call the lock() and unlock()
+// routines around all hashtable operations and subsequent manipulation of
----------------
Again, I don't like that there's a conditional branch throughout the class based on the field.

The most logical way to me would be to define HashTable without any synchronization (i.e. ExternalLock = true), and create a SynchronizedHashTable : public HashTable with corresponding overrides.


https://reviews.llvm.org/D22681