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

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

dexonsmith 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();
+  }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106910