Add a TagSet class providing equality and hashing for key/value maps.#95
Conversation
isturdy
force-pushed
the
tagset
branch
8 times, most recently
from
45df5b4 to
b83e0da
Compare
| TEST(TagSetTest, HashDisregardsOrder) { | ||
| TagSet ts1({{"k1", "v1"}, {"k2", "v2"}}); | ||
| TagSet ts2({{"k2", "v2"}, {"k1", "v1"}}); | ||
| EXPECT_EQ(TagSet::Hash()(ts1), TagSet::Hash()(ts2)); |
There was a problem hiding this comment.
This isn't testing the hash, this is testing that both tagsets are sorted in Initialize()
There was a problem hiding this comment.
Given the implementation--I want it to catch anything silly (such as hashing before sorting).
| // internally because c++ cannot deduce the conversion needed. | ||
| TagSet(std::initializer_list<std::pair<absl::string_view, absl::string_view>> | ||
| tags); | ||
| // This constructor is needed so that callers can dynamically construct |
There was a problem hiding this comment.
How about a single constructor that takes an absl::Span?
There was a problem hiding this comment.
That cannot autoconvert from e.g. {{"key", "value"}}--the compiler can convert the string literal to a string_view or the initialize list to a span, but not both.
There was a problem hiding this comment.
No problem--absl::Span is what I initially wrote too. :)
| } | ||
| } | ||
|
|
||
| std::size_t TagSet::Hash::operator()(const TagSet& tag_set) const { |
There was a problem hiding this comment.
Is this functor is so that we can have an unordered_map? Should there be a test for that?
isturdy
force-pushed
the
tagset
branch
2 times, most recently
from
10b23dd to
ac29491
Compare
| std::hash<std::string> hasher; | ||
| hash_ = 1; | ||
| for (const auto& tag : tags_) { | ||
| static const size_t kMul = static_cast<size_t>(0xdc3eb94af8ab4c93ULL); |
There was a problem hiding this comment.
Let's leave this as-is for now.
But in a followup PR, please factor this against common/internal/string_vector_hash.h. I'd like to have a single place where this hash function (and its magic number) lives. :)
2 participants
This is needed for the DeltaProducer--the ViewData maps include only the tag values in ViewDescriptor order, but since the DeltaProducer aggregates before view aggregation we need to keep the entire mapping.