[PATCH] D16325: Add support for computing SHA1 in LLVM

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 20 07:52:30 PST 2016


joker.eph added a comment.

Thanks for the review.
I don't mind waiting for the use case to be ready before landing this.


================
Comment at: include/llvm/Support/SHA1.h:34
@@ +33,3 @@
+  /// Digest more data.
+  void write(const char *data, size_t len);
+
----------------
filcab wrote:
> I think usually (confirmed in OpenSSL and CommonCrypto) these functions are called Init(), Update(), and Final().
> I don't care about the case, but we should probably use the same words (just like MD5.h is doing).
Didn't know, I'll update.

================
Comment at: include/llvm/Support/SHA1.h:38
@@ +37,3 @@
+  /// last call to init()
+  StringRef result();
+
----------------
filcab wrote:
> We probably want to tell people to not reuse SHA1 objects, and might want to assert in debug mode.
> After all, you could end up hashing weird things like the message from the first call to result() + some padding (and not any bytes you added later).
> 
> With this, you could even re-use the internal state for the result, but that might not be worth it :-)
You mean there is no interest in supporting this:

```
TEST(raw_sha1_ostreamTest, Intermediate) {
  llvm::raw_sha1_ostream Sha1Stream;
  Sha1Stream << "Hello";
  auto Hash = toHex(Sha1Stream.sha1());

  ASSERT_EQ("F7FF9E8B7BB2E09B70935A5D785E0CC5D9D0ABF0", Hash);

  Sha1Stream << " World!";
  Hash = toHex(Sha1Stream.sha1());

  ASSERT_EQ("2EF7BDE608CE5404E97D5F042F95F89F1C232871", Hash);
}
```

right?
Is it because it is potentially allowing misuse?

================
Comment at: lib/Support/SHA1.cpp:29
@@ +28,3 @@
+#elif defined __LITTLE_ENDIAN__
+/* override */
+#elif defined __BYTE_ORDER
----------------
filcab wrote:
> Nit: What does "override" mean in this context? Maybe "default" or "we assume little endian" or something else?
All this file is almost copy pasted (and moved to a LLVM style) : http://llvm.org/docs/doxygen/html/FuzzerSHA1_8cpp_source.html


http://reviews.llvm.org/D16325





More information about the llvm-commits mailing list