[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