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

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

No blocks on my side. Thank you.

P.S: It seems I had a comment to be submitted for a few weeks... Sorry for the lag. :-(


================
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)
Sorry, didn't notice that line before pad() (and the one that restores it).

Thanks!

================
Comment at: lib/Support/SHA1.cpp:166
@@ +165,3 @@
+
+  // Return pointer to hash (20 characters)
+  return Hash;
----------------
Nit: 20 bytes (not "characters"), since it's just the raw bits.


http://reviews.llvm.org/D16325