[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