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

[Alexandre Rames via Phabricator via llvm-commits]

arames added inline comments.


================
Comment at: llvm/include/llvm/Support/HashBuilder.h:405
+
+class HashCodeHasher {
+ public:
----------------
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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109024