[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

Argyrios Kyrtzidis via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 4 21:38:54 PDT 2022

akyrtzi added inline comments.

Comment at: llvm/include/llvm/Support/BLAKE3.h:38
+/// A class that wraps the BLAKE3 algorithm.
+template <size_t NumBytes = LLVM_BLAKE3_OUT_LEN> class BLAKE3 {
dexonsmith wrote:
> The visual noise of `BLAKE3<>` everywhere is a bit unfortunate.
> Another option:
> ```
> lang=c++
> // Hardcoded to 32B. OR same implementation as before, with optional template
> // parameters to choose a size when accessing the hash.
> class BLAKE3 { /* ... */ };
> // Wrap final(), result(), and hash() to truncate the hash.
> template <size_t TruncatedNumBytes> class TruncatedBLAKE3 : public BLAKE3 { ... };
> ```
Good idea! I moved `BLAKE3` back to templated sizes for `final()` and `result()` and added `TruncatedBLAKE3` that has the size parameter on the class.
`TruncatedBLAKE3` is useful for using BLAKE3 as the hasher type for `HashBuilder` with non-default hash sizes, otherwise one can use `BLAKE3` for all other uses.

Comment at: llvm/include/llvm/Support/MD5.h:82
+  /// Finishes off the hash, and returns the 16-byte hash data.
+  std::array<uint8_t, 16> final();
dexonsmith wrote:
> Should this (and `result()` below) be `MD5Result`? It has an implicit cast to `std::array<uint8_t, 16>`. Maybe it's better as you have it... not sure...
I avoided `MD5Result` due to not being as good as `std::array` for a "drop-in replacement" for `StringRef` due to lack of `data()` and `size()`.
But it occurred to me that `MD5Result` could just inherit from `std::array<uint8_t, 16>` which would make it better fit to replace `StringRef` //and// improves & simplifies its API, see updated patch.

Comment at: llvm/lib/Support/SHA1.cpp:289
+    std::array<uint32_t, HASH_LENGTH / 4> HashResult;
+    std::array<uint8_t, 20> ReturnResult;
+  };
dexonsmith wrote:
> Should this be `HASH_LENGTH` instead of 20?
Updated 👍

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list