-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Performance] Remove phmap
dependency.
#7658
Conversation
To trigger regression tests:
|
phmap
with better alternative.phmap
dependency.
LGTM, please keep an eye on next regression result, which will be send later today. |
template <typename K, typename V> | ||
using map_t = tsl::robin_map<K, V>; | ||
template <typename iterator> | ||
auto& mutable_value_ref(iterator it) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why wrap the value access with a function? Is it necessary? It's different from std::unordered_map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it->second is const with this map implementation by default. Use .value() to modify it. I wrapped it so that when we change map again for any reason, we have to only change this function.
Description
@frozenbugs I went ahead and dropped phmap as a dependency as you requested. Please let me know if the PR has can be improved. I am merging so that I can delete the phmap folder from my repo.
Checklist
Please feel free to remove inapplicable items for your PR.
Changes