[PATCH] D109024: [Support] Automatically support `hash_value` when `HashBuilder` support is available.

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

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:92-97
+  /// Trait to indicate whether a type's bits can be hashed directly (after
+  /// endianness correction).
+  template <typename U>
+  struct IsHashableData
+      : std::integral_constant<bool, is_integral_or_enum<U>::value> {};
+
----------------
Noticing now that this trait is independent of HashBuilder's template parameters. I suggest moving it outside HashBuilder in a prep commit (probably doesn't need a review), maybe into a detail namespace like `llvm::hashbuilder_detail`.


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:405
+
+class HashCodeHasher {
+ public:
----------------
arames wrote:
> This was suggested as part of https://reviews.llvm.org/D102943.
> 
> A couple things to consider:
> 
> The default implementation of `hash_value` would always go through `ArrayRef<uint8_t>`:
> This may be less efficient than an explicit implementation. IMO that's ok, since it can be explicitly implemented if performance is an issue.
> The behavior may differ from what would happen with `hash_value` and co. For example `hash_value` for integral and enum types casts to `uint64_t` to guarantee `(char)1` and `(int)1` have the same hash. Again, IMO that's ok. We are talking about generating hashes for custom structures. If this behavior is required, it can be explicitly implemented.
> 
> Using `hash_value` with that mechanism requires calling `llvm::hash_value` or (`using llvm::hash_value`) explicitly. I need to brush up on ADL and template functions (I think that's what involved here). Again, that looks acceptable to me.
> 
> What do you think ?
One option would be to throw `HashCodeHasher` and `HashCodeHashBuilder` into a detail namespace to discourage anyone trying to use `HashCodeHashBuilder` directly. There's an asymmetric relationship:
- Objects get `hash_value` for free if they add `HashBuilder` support.
- ... but using `HashBuilder<HashCodeHasher>` directly isn't very useful, since `HashCodeHashBuilder().add(X).getHasher().Code` is not guaranteed to match `hash_value(X)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109024