Page MenuHomePhabricator

Add hashing support for std::tuple
ClosedPublic

Authored by MForster on Jul 15 2020, 10:15 AM.

Diff Detail

Event Timeline

MForster created this revision.Jul 15 2020, 10:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 10:15 AM

This is a prerequisite for future changes that I am making. The implementation is supposed to be generic and reusable.

gribozavr2 added inline comments.Jul 15 2020, 1:37 PM
llvm/include/llvm/ADT/Hashing.h
125

Could you move it right next to the implementation of hash_value for std::pair above?

713

Ditto, would be nice to maintain the same order as in the header file (right next to the std::pair overload).

716

Could you add a TODO to replace this custom machinery with a call to std::apply when LLVM starts using C++17?

Right now in C++14 I think we can use std::index_sequence_for instead of MakeTupleIndexSet. The helper can also be moved from a side namespace into a local lambda.

llvm/unittests/ADT/HashingTest.cpp
84

Could you imitate these tests for std::tuple as well?

MForster updated this revision to Diff 278377.Jul 15 2020, 11:30 PM
MForster marked 4 inline comments as done.

Address review comments.

MForster added inline comments.Jul 15 2020, 11:32 PM
llvm/include/llvm/ADT/Hashing.h
716

Added a TODO and used std::index_sequence_for. That's neat.

The helper can also be moved from a side namespace into a local lambda.

How would you do that? I know that you can use auto to create a generic lambda, but how do you destructure the type to extract the Indices type parameter?

MForster marked an inline comment as not done.Jul 15 2020, 11:33 PM
MForster updated this revision to Diff 278380.Jul 15 2020, 11:37 PM
This comment was removed by MForster.
MForster updated this revision to Diff 278381.Jul 15 2020, 11:38 PM
This comment was removed by MForster.
MForster updated this revision to Diff 278385.Jul 16 2020, 12:04 AM
This comment was removed by MForster.
gribozavr2 accepted this revision.Jul 16 2020, 3:57 AM
gribozavr2 marked an inline comment as done.
gribozavr2 added inline comments.
llvm/include/llvm/ADT/Hashing.h
716

Ah right, we need a named template parameter type pack.

This revision is now accepted and ready to land.Jul 16 2020, 3:57 AM
This revision was automatically updated to reflect the committed changes.