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

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.
filcab added a comment.

I have some minor comments.
I think having a patch using this before it lands would be nice. :-)

Thank you for working on this.


================
Comment at: include/llvm/Support/SHA1.h:34
@@ +33,3 @@
+  /// Digest more data.
+  void write(const char *data, size_t len);
+
----------------
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).

================
Comment at: include/llvm/Support/SHA1.h:38
@@ +37,3 @@
+  /// last call to init()
+  StringRef result();
+
----------------
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 :-)

================
Comment at: lib/Support/SHA1.cpp:29
@@ +28,3 @@
+#elif defined __LITTLE_ENDIAN__
+/* override */
+#elif defined __BYTE_ORDER
----------------
Nit: What does "override" mean in this context? Maybe "default" or "we assume little endian" or something else?

================
Comment at: lib/Support/SHA1.cpp:30
@@ +29,3 @@
+/* override */
+#elif defined __BYTE_ORDER
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
----------------
In the compilers we support, do we need to resort to this? I don't know about MSVC, but IIRC, gcc and clang always define __{BIG,LITTLE}_ENDIAN__, no?

We might not need so many tests.

================
Comment at: lib/Support/SHA1.cpp:45
@@ +44,3 @@
+#define SHA1_K40 0x8f1bbcdc
+#define SHA1_K60 0xca62c1d6
+
----------------
Nit: Why do the round constants get names, but the seeds don't?

================
Comment at: lib/Support/SHA1.cpp:120
@@ +119,3 @@
+void SHA1::write(const char *data, size_t len) {
+  for (; len--;)
+    writebyte((uint8_t)*data++);
----------------
Nit:
  while(len--) //?


================
Comment at: lib/Support/SHA1.cpp:143
@@ +142,3 @@
+}
+#include <cstdio>
+StringRef SHA1::result() {
----------------
Why here?

================
Comment at: lib/Support/SHA1.cpp:150
@@ +149,3 @@
+
+#ifndef SHA_BIG_ENDIAN
+  // Swap byte order back
----------------
Nit: I'd prefer a positive test (`ifdef`, like the one in line 102)


http://reviews.llvm.org/D16325