[PATCH] D83887: Add hashing support for std::tuple

[Dmitri Gribenko via Phabricator via llvm-commits]

gribozavr2 added inline comments.


================
Comment at: llvm/include/llvm/ADT/Hashing.h:121
+/// Compute a hash_code for a tuple.
+template <typename... Ts> hash_code hash_value(const std::tuple<Ts...> &arg);
 
----------------
Could you move it right next to the implementation of hash_value for std::pair above?


================
Comment at: llvm/include/llvm/ADT/Hashing.h:691
+
+template <typename... Ts> hash_code hash_value(const std::tuple<Ts...> &arg) {
+  typename ::llvm::hashing::detail::MakeTupleIndexSet<0, sizeof...(Ts)>::Type
----------------
Ditto, would be nice to maintain the same order as in the header file (right next to the std::pair overload).


================
Comment at: llvm/include/llvm/ADT/Hashing.h:694
+      indices;
+  return hash_value_tuple_helper(arg, indices);
+}
----------------
Could you add a TODO to replace this custom machinery with a call to std::apply when LLVM starts using C++17?

Right now in C++14 I think we can use `std::index_sequence_for` instead of `MakeTupleIndexSet`. The helper can also be moved from a side namespace into a local lambda.


================
Comment at: llvm/unittests/ADT/HashingTest.cpp:84
   EXPECT_NE(hash_combine(42, 43), hash_value(std::make_pair(42, 43ull)));
   EXPECT_NE(hash_combine(42, 43), hash_value(std::make_pair(42ull, 43)));
 
----------------
Could you imitate these tests for std::tuple as well?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83887/new/

https://reviews.llvm.org/D83887