SafeUnorderedMap fixes #99
Conversation
- new custom iterators keep all internals hidden - remove range-erase erase(first, last) method (use it = erase(it) in a loop instead) - remove all node_type methods (harder to implement now that internals are not exposed) - remove insert(T&&) template methods (don't instantiate) - add testcases for functionality tests and for template instantiation tests
- Keep size_ valid in operator[] and erase() at no extra cost - Keep size_ valid in insert(pair) and insert_or_assign(k,v) at the cost of two extra hash lookups (map_ & erasedKeys_), miminizing odds that a call to size() would have to count elements, which can be expensive; better to pay the (constant) cost upfront as the default (can be improved later by e.g. adding a configuration bool to control this behavior) - Added a few size() checks to tests
| check(); | ||
| auto mapPtrIt = mapPtr_->find(std::forward<K>(key)); | ||
| if (mapPtrIt != mapPtr_->end()) { | ||
| mapPtr_->erase(std::forward<K>(key)); |
There was a problem hiding this comment.
key might dangle since you already forwarded it in line 628. We should std::forward a forwarding reference only once. Do you think this overload is truly necessary? It will be a bit tricky to ensure a single std::forward as we track erased keys.
There was a problem hiding this comment.
By the way, why is this overload so different from the erase(const Key& key)?
There was a problem hiding this comment.
Yep, I also understand that std::forward should be used only once, though I don't know if there's some kind of nuance here being exploited. This was the previous behavior (if I'm not mistaken). I tried to mimic as much of the previous code as possible. I'm down with removing this overload (and perhaps others) if people want to.
EDIT: Let's just remove this overload.
| std::pair<typename std::unordered_map<Key, T, SafeHash>::iterator, bool> insert( | ||
| typename std::unordered_map<Key, T, SafeHash>::value_type&& value | ||
| std::pair<typename SafeUnorderedMap<Key, T>::iterator, bool> insert( | ||
| const typename SafeUnorderedMap<Key, T>::value_type&& value |
There was a problem hiding this comment.
const here is preventing moving. Remember that moving A to B requires modifying A, so it can't be const. The current code will actually cause a copy.
|
This is obsoleted by some radical changes coming to the Safe Variables feature. I'll keep this branch for now as a source of ideas for upcoming work. |
This patch hides the
std::unordered_mapiterators that were exposed, and replaces them with newSafeUnorderedMap::iteratorandSafeUnorderedMap::const_iterator.As part of the proposed changes, some
SafeUnorderedMapmethods had to be removed. We can re-implement them later without exposing internal details, if it turns out that we will be needing them.Added tests for the new features, and also some additional tests to make sure all of the insert/emplace/erase methods from the template type get instantiated at least once.