[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