[PATCH] D106910: [Support]: Introduce the `HashBuilder` interface.

[Alexandre Rames via Phabricator via llvm-commits]

arames added inline comments.


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:60-66
+  template <typename T> using HasFinalT = decltype(std::declval<T &>().final());
+  /// Forward to `HasherT::final()` if available.
+  template <typename HasherT_ = HasherT>
+  std::enable_if_t<is_detected<HasFinalT, HasherT_>::value, HasFinalT<HasherT_>>
+  final() {
+    return this->getHasher().final();
+  }
----------------
dexonsmith wrote:
> I don't think we need enable_if or return type deduction here. It seems like overkill to make "does HashBuilder::final work?" SFINAE-detectible.
You may want to double-check my earlier comments.
I think there is a strong value in allowing trivial support for user-defined (or simply other) hashes. Here, that means:
* not requiring support for `HasherT::final()`
* not assuming the return type


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:100-102
+  using Base =
+      HashBuilderBase<HashBuilder, HasherT, Endianness, EffectiveEndianness>;
+  using Derived = HashBuilder<HasherT, Endianness, EffectiveEndianness>;
----------------
dexonsmith wrote:
> Using CRTP in `HashBuilderImpl` also increases compile-time / code size since it blocks sharing code between "native" and whatever the system endianness is. I suggest dropping CRTP and returning `HashBuilderImpl&` instead of `HashBuilder&` from `add*`. (If this needed CRTP, which I don't think it does, I'd suggest using an opaque `DerivedT` to avoid passing through details like EffectiveEndianness.)
> 
Removed CRTP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106910