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

[Alexandre Rames via Phabricator via cfe-commits]

arames marked 2 inline comments as done.
arames added inline comments.


================
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) {
----------------
dexonsmith wrote:
> 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...
I created https://reviews.llvm.org/D109024 for this.
If we decide to go for that mechanism, whichever PR lands first, I can update the other to use the new mechanism.



================
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 ?
----------------
dexonsmith wrote:
> I'd simplify:
> ```
> lang=c++
> // FIXME: Consider using SHA1 instead of MD5.
> ```
Do you think we should switch to `SHA1` now ?
I picked MD5 at random amongst the available hashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102943