This is an archive of the discontinued LLVM Phabricator instance.

[Support] Automatically support `hash_value` when `HashBuilder` support is available.
ClosedPublic

Authored by arames on Aug 31 2021, 1:57 PM.

Details

Summary

Use the HBuilder interface to provide default implementations of llvm::hash_value.

Diff Detail

Event Timeline

arames created this revision.Aug 31 2021, 1:57 PM
arames requested review of this revision.Aug 31 2021, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 1:57 PM
arames edited the summary of this revision. (Show Details)Aug 31 2021, 2:04 PM
arames added inline comments.Aug 31 2021, 2:16 PM
llvm/include/llvm/Support/HashBuilder.h
405

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 ?

arames updated this revision to Diff 369805.Aug 31 2021, 2:59 PM

Fix clang-format warning.

dexonsmith added inline comments.Sep 2 2021, 12:53 PM
llvm/include/llvm/Support/HashBuilder.h
92–97

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.

405

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).
arames updated this revision to Diff 370431.Sep 2 2021, 3:40 PM
arames marked 2 inline comments as done.

Address review comments.

arames added a comment.Sep 2 2021, 3:40 PM

Applied suggestions.

dexonsmith added inline comments.Sep 2 2021, 3:48 PM
llvm/include/llvm/Support/HashBuilder.h
30–37

This change should be a separate NFC commit, since it touches a lot of lines (and isn't directly related)

82

Diff would be reduced if teh public: didn't move before teh static_assert

405

Probably cleaner to use the same name for the detail namespace as above.

arames updated this revision to Diff 370444.Sep 2 2021, 4:25 PM
arames marked 3 inline comments as done.

Address review comments.

arames added inline comments.Sep 3 2021, 11:44 AM
llvm/include/llvm/Support/HashBuilder.h
30–37
dexonsmith accepted this revision.Sep 7 2021, 7:58 AM

LGTM, with one more inline comment.

llvm/include/llvm/Support/HashBuilder.h
419–420

Please move this typedef into the detail namespace as well, since it's not intended to be used directly.

Could even be inside HashCodeHasher:

class HashCodeHasher {
public:
  //...

  using Builder = HashBuilder<...>;
};

but up to you.

This revision is now accepted and ready to land.Sep 7 2021, 7:58 AM
This revision was landed with ongoing or failed builds.Sep 7 2021, 9:56 AM
This revision was automatically updated to reflect the committed changes.