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

[Derek Bruening via llvm-commits]

bruening marked 2 inline comments as done.

================
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.
----------------
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.

================
Comment at: lib/esan/esan_hashtable.h:42
@@ +41,3 @@
+  ~HashTable();
+  bool lookup(KeyTy Key, DataTy &Payload);
+  bool add(KeyTy Key, DataTy Payload);
----------------
aizatsky wrote:
> aizatsky wrote:
> > any particular reason why not
> > 
> > DataTy* lookup(KeyTy Key) ?
> > 
> > Seems to me it would always save a declaration line for clients.
> const
> any particular reason why not
> DataTy* lookup(KeyTy Key) ?

Returning a pointer requires external synchronization.  An instantiation with DataTy==int will want an int to be returned by value, and there's no sentinel we can use to indicate failure.



================
Comment at: lib/esan/esan_hashtable.h:42
@@ +41,3 @@
+  ~HashTable();
+  bool lookup(KeyTy Key, DataTy &Payload);
+  bool add(KeyTy Key, DataTy Payload);
----------------
bruening wrote:
> aizatsky wrote:
> > aizatsky wrote:
> > > any particular reason why not
> > > 
> > > DataTy* lookup(KeyTy Key) ?
> > > 
> > > Seems to me it would always save a declaration line for clients.
> > const
> > any particular reason why not
> > DataTy* lookup(KeyTy Key) ?
> 
> Returning a pointer requires external synchronization.  An instantiation with DataTy==int will want an int to be returned by value, and there's no sentinel we can use to indicate failure.
> 
> 
> const

I ended up removing my initial const because of the potential lock acquisition: the mutex will change state.  I don't think LLVM supports the C++11 mutable keyword?

================
Comment at: lib/esan/esan_hashtable.h:45
@@ +44,3 @@
+  bool remove(KeyTy Key);
+  u32 size();
+  // If the table is internally-synchronized, this lock must not be held
----------------
aizatsky wrote:
> const
Same thing here.  I can at least put a comment about it.


https://reviews.llvm.org/D22681