[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:
> arames wrote:
> > 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
> On not requiring HasherT::final, my suggested edit doesn't change that (unless I'm missing something?). The call to HasherT::final is template-dependent. Both before and after the edit, there will be no compile errors if there are no calls to `HashBuilder<HasherT>::final`, and a compile error if there is such a call. The only semantic difference is whether HashBuilder::final is SFINAE-detectable (compile error because there is no overload for HashBuilder::final vs. compile error because the overload for HashBuilder::final can't be instantiated), which doesn't seem important.
> 
> On the return type, I don't see the benefit of allowing flexibility here. I also don't see a tie-in to the argument about trivial support. And there is a cost: generic code that depends on HashBuilder::final will have to reason about other arbitrary return types. It's simpler for clients if this just returns `StringRef`.
> 
> If you still think there's strong value in either of those (which both seem unrelated to allowing use of trivial HasherT types), can you clarify what it is?
Both done.  (will be updated in the next `arc diff`.)

I misunderstood your earlier comment, thinking you suggested to still require `HasherT::final()`. And I didn't think of simply relying on the template for the purpose.
I felt specializing the interface to follow particular hasher types might not be very wise. I could imagine a hash wanted to return say `uint64_t`. But since this is hypothetical, I buy the argument of just keeping things simple for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106910