[PATCH] D19045: Add a CachedHash structure

[David Blaikie via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good apart from some optional feedback - thanks!


================
Comment at: include/llvm/ADT/DenseMapInfo.h:34-35
@@ +33,4 @@
+template <typename T> struct CachedHash {
+  CachedHash(T Val) : Val(Val) { Hash = DenseMapInfo<T>::getHashValue(Val); }
+  CachedHash(T Val, unsigned Hash) : Val(Val), Hash(Hash) {}
+  T Val;
----------------
I think you can remove both of these constructors if you're going to use braced init anyway?

(though I guess maybe you need the first because you want implicit conversion from T -> CachedHash)

Also, any desire to make this do the right thing for movable types? I'd probably at least add the "Val(std::move(Val))" in the ctors (whichever ones you keep, if any). That still won't be great for expensive-to-move things, but they're less of a priority & hopefully we just fix them.

================
Comment at: unittests/ADT/DenseMapTest.cpp:512
@@ +511,3 @@
+  static unsigned getHashValue(const CachedHashTest &X) {
+    (*X.Counter)++;
+    return X.Val;
----------------
Could just write this as ++*X.Counter if you like. Up to you.


http://reviews.llvm.org/D19045