[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

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

dexonsmith added a comment.

A couple of suggestions inline, but this basically LGTM.

@jansvoboda11 @Bigcheese : can you take a look as well and confirm this looks reasonable from your perspectives?



================
Comment at: clang/include/clang/Basic/ObjCRuntime.h:486
+  template <typename HasherT, llvm::support::endianness Endianness>
+  friend void addHash(llvm::HashBuilderImpl<HasherT, Endianness> &HBuilder,
+                      const ObjCRuntime &OCR) {
----------------
arames wrote:
> I have added these in the same line as existing `hash_value` functions. The idea being others may also need to hash those objects.
> Let me know if you would rather move some/all of these locally in `CompilerInvocation.cpp`.
Seems reasonable to me.

An option to consider (maybe in a separate commit?) would be to add:
```
lang=c++
class HashCodeHasher {
public:
  void update(ArrayRef<uint8_t> Bytes) {
    llvm::hash_code BytesCode = llvm::hash_value(Bytes);
    Code = Code ? BytesCode : llvm::hash_combine(*Code, BytesCode);
  }
  Optional<llvm::hash_code> Code;
};
template <class T,
          std::enable_if_t<is_hashable<T>::value, bool> = true>
llvm::hash_code hash_value(const T &Value) {
  HashBuilder<HashCodeHasher, support::endianness::native> Builder;
  Builder.add(Value);
  return *Builder.Code;
}
```
Then objects that support HashBuilder also support `llvm::hash_value` without extra code...


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4473-4475
+  // FIXME: We want to use something cryptographically sound. This was
+  // initially using CityHash (via `llvm::hash_code`). We moved to `llvm::MD5`.
+  // Is thie appropriate ?
----------------
I'd simplify:
```
lang=c++
// FIXME: Consider using SHA1 instead of MD5.
```


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:167
   /// template <typename HasherT, support::endianness Endianness>
-  /// void addHash(HashBuilder<HasherT, Endianness> &HBuilder,
+  /// void addHash(HashBuilderImpl<HasherT, Endianness> &HBuilder,
   ///              const UserDefinedStruct &Value);
----------------
Please fix in a separate commit (no need for a review IMO).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102943