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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Thanks for all the comments, I tried to address them. 
As long as we're OK with the API, we'll still be able to improve the implementation later performance-wise if necessary.


================
Comment at: include/llvm/Support/SHA1.h:38
@@ +37,3 @@
+  /// last call to init()
+  StringRef result();
+
----------------
joker.eph wrote:
> filcab wrote:
> > joker.eph wrote:
> > > 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?
> > As written we don't support that (you'll addUncounted one bit, and then the padding. Afterwards, adding to the stream won't do what it would need to do). I'd like that to be explicit in the documentation (and I'd like an assert or two to make it trigger in debug mode, if that's attempted).
> I'm not sure what you mean, the InternalState is saved and restored in `SHA1::result() ` so that we can resume as is. (this unittest is passing)
support both behavior through different API calls now.

================
Comment at: lib/Support/SHA1.cpp:71
@@ +70,3 @@
+  for (i = 0; i < 80; i++) {
+    if (i >= 16) {
+      t = InternalState.Buffer[(i + 13) & 15] ^
----------------
deadalnix wrote:
> I would do 2 loops
not sure what you mean?


http://reviews.llvm.org/D16325