Cleanup - #13
Conversation
There is an issue with the implementation of potentially modifying methods: accessing or reassigning an existing value (means no insertion) may trigger a hash table rehash. This is because load factor thresholds are checked unconfitionally in the common insertion routine. Such behaviour leads to a violation of the following iterator invalidation contract: > - insert, emplace, emplace_hint, operator[]: if there is an effective > insert, invalidate the iterators. User code, being unaware of this bug, may suffer from use-after-free. Fix the issue by moving threshold checks inside the insertion subroutine.
bigerl
left a comment
bigerl
left a comment
There was a problem hiding this comment.
Partial Review sparse_hash.hpp (1)
| cmake-build-type: Release | ||
| } | ||
| name: ${{matrix.config.name}} | ||
| # These get queued but never actually run |
There was a problem hiding this comment.
When merged, we should add issues with tag help-wanted to add windows and macos support to the CI.
| * Maximum that size_ can reach before a rehash occurs automatically | ||
| * to grow the hash table. | ||
| */ | ||
| size_type load_threshold_rehash_; |
There was a problem hiding this comment.
Does this (and the value below) really make a difference? Is it measurable slower if we calculate it on the fly?
There was a problem hiding this comment.
Yes a little bit
Cached as member:
Calculated on the fly:
| hasher const &hash, | ||
| key_equal const &equal, | ||
| allocator_type const &alloc) : h_{hash}, | ||
| keq_{equal}, |
There was a problem hiding this comment.
Does no_unique_address + this result in a no-op?
There was a problem hiding this comment.
I think this is optimizer dependent. In theory the ctor could still do something, even if it isn't initializing the object.
| return erase(mutable_iterator(pos)); | ||
| } | ||
|
|
||
| iterator erase(const_iterator first, const_iterator last) { |
There was a problem hiding this comment.
This whole function doesn't make much sense to me. Erase can trigger a rehash, right? So we cannot even be sure that the range of entries that the user could observe before the call between first and last is even correctly erase.
There was a problem hiding this comment.
Yeah I was wondering the same thing. I honestly don't know if erasing ranges from hash maps ever makes sense, I mean you don't even know whats in the range in the first place as its effectively random.
Just remove?
There was a problem hiding this comment.
I would keep the function but deactivate it with an requires false + an explanation why it is not supported ATM.
There was a problem hiding this comment.
Actually no, it cannot trigger a rehash, because you couldn't do erase loops if it did.
But the function is still pretty useless I would say.
| } | ||
|
|
||
| template<typename K> | ||
| size_type erase(K const &key) { |
3 participants
No description provided.