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

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

Thanks for the working through this! LGTM if you remove the CRTP (see below, I don't think it's needed). See also a few more nits I found to pick.



================
Comment at: llvm/include/llvm/Support/HashBuilder.h:30-35
+template <template <typename, support::endianness, support::endianness>
+          class HashBuilder,
+          class HasherT, support::endianness Endianness,
+          support::endianness EffectiveEndianness>
+class HashBuilderBase {
+  using Derived = HashBuilder<HasherT, Endianness, EffectiveEndianness>;
----------------
Using CRTP blocks the compile-time speedup / code size wins. Can we skip HashBuilder/Endianness/EffectiveEndianness/Derived?


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:45-48
+  auto &update(ArrayRef<uint8_t> Data) {
+    this->getHasher().update(Data);
+    return *static_cast<Derived *>(this);
+  }
----------------
I suggest dropping the return statement, for `void update(ArrayRef)`, a simple passthrough to HasherT::update.
- No need for the CRTP (faster compile-time / avoid code size bloat by factoring out a common instantiation for the base class)
- The same functionality is available as `HashBuilder& HasherBuilder::addRangeElements`, so there's no loss of convenience


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:55
+  /// contraints.
+  auto &update(StringRef Data) {
+    return update(makeArrayRef(reinterpret_cast<const uint8_t *>(Data.data()),
----------------
(this would also return `void`)


================
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();
+  }
----------------
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.


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:68-76
+  template <typename T>
+  using HasResultT = decltype(std::declval<T &>().result());
+  /// Forward to `HasherT::result()` if available.
+  template <typename HasherT_ = HasherT>
+  std::enable_if_t<is_detected<HasResultT, HasherT_>::value,
+                   HasResultT<HasherT_>>
+  result() {
----------------
Same as `final`.


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:91
+
+/// Implementation of the `HashBuilder` interface.
+template <template <typename, support::endianness, support::endianness>
----------------
I think this comment should explain that `support::endianness::native` is not supported here but that it's handled by `HashBuilder`. Basically, explain the static assertion in text.


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:100-102
+  using Base =
+      HashBuilderBase<HashBuilder, HasherT, Endianness, EffectiveEndianness>;
+  using Derived = HashBuilder<HasherT, Endianness, EffectiveEndianness>;
----------------
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.)



================
Comment at: llvm/include/llvm/Support/HashBuilder.h:410-424
+template <typename HasherT, support::endianness Endianness,
+          support::endianness EffectiveEndianness =
+              (Endianness == support::endianness::native
+                   ? support::endian::system_endianness()
+                   : Endianness)>
+class HashBuilder : public HashBuilderImpl<HashBuilder, HasherT, Endianness,
+                                           EffectiveEndianness> {
----------------
Given that `add*` will return `HashBuilderImpl&`, might be reasonable to reduce this to a typedef:
```
lang=c++
template <class HasherT, support::endianness Endianness>
using HashBuilder =
    HashBuilderImpl<HasherT,
                    (Endianness == support::endianness::native
                         ? support::endian::system_endianness()
                         : Endianness)>;
```
Either way seems fine though.


================
Comment at: llvm/unittests/Support/HashBuilderTest.cpp:46-50
+template <typename HasherTAndEndianness, typename... Ts>
+static auto hashWithBuilder(const Ts &...Args) {
+  return static_cast<std::string>(
+      HashBuilder<HasherTAndEndianness>().add(Args...).final());
+}
----------------
I'd skip `auto` type deduction unless the type is hard to spell. Also you can use StringRef::str here.


================
Comment at: llvm/unittests/Support/HashBuilderTest.cpp:53-56
+static auto hashRangeWithBuilder(const Ts &...Args) {
+  return static_cast<std::string>(
+      HashBuilder<HasherTAndEndianness>().addRange(Args...).final());
+}
----------------
(same as above)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106910