[PATCH] D19045: Add a CachedHash structure

Rafael EspĂ­ndola via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 21 05:10:27 PDT 2016


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

Correct. And once you have the first you need the second.

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

Done.

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

Done.

Cheers,
Rafael


More information about the llvm-commits mailing list