Skip to content

Conversation

@kazutakahirata
Copy link
Contributor

We just started using a C++17 fold expression inside combine
(#159901). We still carry the style that was suitable for recursive
calls of combine_data. Specifically, we keep passing several state
variables as parameters of combine_data.

Now that we no longer use recursion, this patch simplifies
hash_combine_recursive_helper by making buffer_ptr, buffer_end, and
length member variables while dropping the return value from
hash_combine. This patch also names hash_combine_recursive_helper to
hash_combine_helper.

I will follow up to update comments that still mention "recursion".

We just started using a C++17 fold expression inside combine
(llvm#159901).  We still carry the style that was suitable for recursive
calls of combine_data.  Specifically, we keep passing several state
variables as parameters of combine_data.

Now that we no longer use recursion, this patch simplifies
hash_combine_recursive_helper by making buffer_ptr, buffer_end, and
length member variables while dropping the return value from
hash_combine.  This patch also names hash_combine_recursive_helper to
hash_combine_helper.

I will follow up to update comments that still mention "recursion".
@llvmbot
Copy link
Member

llvmbot commented Sep 20, 2025

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

We just started using a C++17 fold expression inside combine
(#159901). We still carry the style that was suitable for recursive
calls of combine_data. Specifically, we keep passing several state
variables as parameters of combine_data.

Now that we no longer use recursion, this patch simplifies
hash_combine_recursive_helper by making buffer_ptr, buffer_end, and
length member variables while dropping the return value from
hash_combine. This patch also names hash_combine_recursive_helper to
hash_combine_helper.

I will follow up to update comments that still mention "recursion".


Full diff: https://github.com/llvm/llvm-project/pull/159938.diff

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/Hashing.h (+13-16)
diff --git a/llvm/include/llvm/ADT/Hashing.h b/llvm/include/llvm/ADT/Hashing.h
index dffe3791e51c7..31d7be2714a20 100644
--- a/llvm/include/llvm/ADT/Hashing.h
+++ b/llvm/include/llvm/ADT/Hashing.h
@@ -483,8 +483,11 @@ namespace detail {
 /// recursive combining of arguments used in hash_combine. It is particularly
 /// useful at minimizing the code in the recursive calls to ease the pain
 /// caused by a lack of variadic functions.
-struct hash_combine_recursive_helper {
+struct hash_combine_helper {
   char buffer[64] = {};
+  char *buffer_ptr;
+  char *const buffer_end;
+  size_t length = 0;
   hash_state state;
   const uint64_t seed;
 
@@ -493,8 +496,9 @@ struct hash_combine_recursive_helper {
   ///
   /// This sets up the state for a recursive hash combine, including getting
   /// the seed and buffer setup.
-  hash_combine_recursive_helper()
-    : seed(get_execution_seed()) {}
+  hash_combine_helper()
+      : buffer_ptr(buffer), buffer_end(buffer + 64),
+        seed(get_execution_seed()) {}
 
   /// Combine one chunk of data into the current in-flight hash.
   ///
@@ -502,9 +506,7 @@ struct hash_combine_recursive_helper {
   /// the data. If the buffer is full, it hashes the buffer into its
   /// hash_state, empties it, and then merges the new chunk in. This also
   /// handles cases where the data straddles the end of the buffer.
-  template <typename T>
-  char *combine_data(size_t &length, char *&buffer_ptr, char *buffer_end,
-                     T data) {
+  template <typename T> void combine_data(T data) {
     if (!store_and_advance(buffer_ptr, buffer_end, data)) {
       // Check for skew which prevents the buffer from being packed, and do
       // a partial store into the buffer to fill it. This is only a concern
@@ -535,19 +537,14 @@ struct hash_combine_recursive_helper {
                              partial_store_size))
         llvm_unreachable("buffer smaller than stored type");
     }
-    return buffer_ptr;
   }
 
   /// Recursive, variadic combining method.
   ///
   /// This function recurses through each argument, combining that argument
   /// into a single hash.
-  template <typename... Ts>
-  hash_code combine(size_t length, char *buffer_ptr, char *buffer_end,
-                    const Ts &...args) {
-    ((void)combine_data(length, buffer_ptr, buffer_end,
-                        get_hashable_data(args)),
-     ...);
+  template <typename... Ts> hash_code combine(const Ts &...args) {
+    (combine_data(get_hashable_data(args)), ...);
 
     // Finalize the hash by flushing any remaining data in the buffer.
     //
@@ -584,10 +581,10 @@ struct hash_combine_recursive_helper {
 /// The result is suitable for returning from a user's hash_value
 /// *implementation* for their user-defined type. Consumers of a type should
 /// *not* call this routine, they should instead call 'hash_value'.
-template <typename ...Ts> hash_code hash_combine(const Ts &...args) {
+template <typename... Ts> hash_code hash_combine(const Ts &...args) {
   // Recursively hash each argument using a helper class.
-  ::llvm::hashing::detail::hash_combine_recursive_helper helper;
-  return helper.combine(0, helper.buffer, helper.buffer + 64, args...);
+  ::llvm::hashing::detail::hash_combine_helper helper;
+  return helper.combine(args...);
 }
 
 // Implementation details for implementations of hash_value overloads provided

@kazutakahirata kazutakahirata merged commit a36a1ec into llvm:main Sep 21, 2025
11 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_20250920_10TMP_ADT_hash_combiner_followup branch September 21, 2025 01:43
kazutakahirata added a commit to kazutakahirata/llvm-project that referenced this pull request Sep 21, 2025
Now that we have removed recursion in llvm#159901 and llvm#159938, this patch
updates those comments that still mention recursion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants