[PATCH] D107781: [Support] Introduce `SmallString<32> MD5::final()`...

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

Sorry I missed this before -- I have a few suggestions below to align this better with SHA1 and SHA256.



================
Comment at: llvm/include/llvm/Support/MD5.h:93-95
+  /// Finishes the hash, and returns the result as a pair of words.
+  std::pair<uint64_t, uint64_t> finalWords();
+
----------------
I'm not seeing `finalWords()` in the other hashers. I suggest not adding it.


================
Comment at: llvm/include/llvm/Support/MD5.h:97
+  /// Finishes the hash, and returns the result as a hex string of length 32.
+  SmallString<32> final();
+
----------------
The other hashers return a `StringRef`. MD5 should align with that, and also return the raw bytes as they do (instead of a hex string).

Looking at SHA1 and SHA256, they have two methods, `result()` and `final()`, the former of which can be called at any time. Here is the implementation of `result()`:
```
StringRef SHA1::result() {
  auto StateToRestore = InternalState;

  auto Hash = final();

  // Restore the state
  InternalState = StateToRestore;

  // Return pointer to hash (20 characters)
  return Hash;
}
```
I think if we're adding one we should add both, for consistency.
- Seems we'd need a prep NFC patch that moved its members into an `InternalState` member.
- `MD5::final()` can add an `MD5Result` member, calling `final(MD5Result&)` on it and returning a `StringRef` referencing `MD5Result::bytes`, and then `MD5::result()` can be implemented exactly as in SHA1.


================
Comment at: llvm/lib/Support/MD5.cpp:276
+  final(Result);
+  return Result.digest();
+}
----------------
The other hashers do not return a digest, but the raw bytes. MD5 should align with that.



================
Comment at: llvm/unittests/Support/MD5Test.cpp:77
+    SmallString<32> Result = Hash.final();
+    const char *Expected = "e2fc714c4727ee9395f324cd2e7f331f";
+    EXPECT_EQ(Result, Expected);
----------------
I think this should avoid a magic number, and just confirm that the `final()` return is equal to `MD5Result::bytes` when calling `final(MD5Result&)`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107781/new/

https://reviews.llvm.org/D107781