[PATCH] D98438: Clang: Allow selecting the hash algorithm for file checksums in debug info.

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Sorry for not seeing this sooner.

I think this looks great, just a few comments, and it also needs tests.



================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:303
+/// Set debug info source file hashing algorithm
+ENUM_CODEGENOPT(DebugSrcHashAlgorithm, SrcHashAlgorithm, 4, CSK_MD5)
+
----------------
Why does it need 4 bits? Wouldn't 2 be enough?


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:368
 
-Optional<llvm::DIFile::ChecksumKind>
-CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
+Optional<llvm::DIFile::ChecksumInfo<StringRef>>
+CGDebugInfo::computeChecksum(FileID FID, SmallString<64> &Checksum) const {
----------------
I'm not sure changing the return type helps that much? I suppose it makes the calling code a little cleaner, but I think it makes the function itself a little more confusing.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:398
+    StringRef Result = Hash.final();
+    llvm::raw_svector_ostream Res(Checksum);
+    for (int i = 0; i < 20; ++i)
----------------
toHex() from StringExtras.h is much nicer, and MD5 should be using that internally too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98438