[PATCH] D121510: [Support] Introduce the BLAKE3 hashing function implementation

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/Support/BLAKE3.h:16
+
+#include "llvm-c/blake3.h"
+#include "llvm/ADT/ArrayRef.h"
----------------
Might be nice to bury this `#include` in the source file to avoid polluting the global namespace with the C stuff. Inlining would still be available for release builds (assuming they use ThinLTO).


================
Comment at: llvm/include/llvm/Support/BLAKE3.h:66-70
+  template <size_t NumBytes = BLAKE3_OUT_LEN> BLAKE3Result<NumBytes> final() {
+    BLAKE3Result<NumBytes> Result;
+    blake3_hasher_finalize(&Hasher, Result.data(), Result.size());
+    return Result;
+  }
----------------
Other hashers (SHA1/MD5/SHA256) return a `StringRef` from the `final()` function. Generic APIs (such as llvm/Support/HashBuilder.h) rely on this being the same everywhere.

Is it reasonable to update this to return a StringRef, or can/should we change the other things to return a `std::array` to match this?

(If returning a StringRef here, we could add a `std::array<uint8_t, 32> Hash` field to BLAKE3, fill it using `blake3_hasher_finalize()`, and then call `toStringRef(makeArrayRef(Hash)).take_front(NumBytes)`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121510