[PATCH] D22512: Added hash_stream class for producing hash codes from data streams.

Artem Dergachev via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 15 07:35:56 PDT 2016


NoQ added inline comments.

================
Comment at: include/llvm/ADT/Hashing.h:37
@@ -36,1 +36,3 @@
 //
+//  -- 'hash_stream' is a class for combining a stream of data into a single
+//     hash_code.
----------------
I think it's worth mentioning that `hash_stream` wasn't proposed as part of N3333, unlikle other classes in this file. Or maybe it could be proposed :)

================
Comment at: include/llvm/ADT/Hashing.h:731
@@ +730,3 @@
+    append_to_buffer(c, std::min(size, remaining_size));
+    if (size <= remaining_size)
+      return *this;
----------------
Maybe compare `size` and `remaining_size` only once? The control flow would also be more clear that way.
```
    if (size <= remaining_size) {
      append_to_buffer(c, size);
      return *this;
    }

    append_to_buffer(c, remaining_size);
```

================
Comment at: unittests/ADT/HashingTest.cpp:160
@@ +159,3 @@
+  // Test that data type size matters for hash_stream.
+  streamA << 56l;
+  streamB << 55;
----------------
Maybe you wanted to use '`55l`' here? (:


https://reviews.llvm.org/D22512





More information about the llvm-commits mailing list