martinus / robin-hood-hashing

Fast & memory efficient hashtable based on robin hood hashing for C++11/14/17/20
https://gitter.im/martinus/robin-hood-hashing
MIT License
1.5k stars 143 forks source link

Creating `std::vector<std::pair<K, V>>` is less than ergonomic #45

Closed jthemphill closed 4 years ago

jthemphill commented 4 years ago

This code compiles for std::unordered_map but not for robin_hood::unordered_map:

#include <utility>

#include "robin_hood.h"

int main() {
  robin_hood::unordered_map<int, int> m;
  std::vector<std::pair<int, int>> rows{m.begin(), m.end()};
}

Replacing std::pair with robin_hood::pair would make the code compile, but would also cause a hashmap implementation's details to propagate throughout the codebase. Replacing this one-liner with a for-loop would also work, but the ergonomics are worse and developers may wind up propagating robin_hood::pairs anyway by following the path of least resistance.

martinus commented 4 years ago

Well, when you use std::pair<int, int> you are already assuming something about the map's internal data structure. I'd say the correct way to do this is something like this:

using Map = robin_hood::unordered_map<int, int>;
Map m;
std::vector<Map::value_type> rows(m.begin(), m.end());

I have refactored most of our code that assumed std::pair internals like this. Also the use of std::pair is often a sign that it's not optimally coded. e.g.

map.insert(std::make_pair<int,int>(123, 321));

creates uselessly temporary objects. It's better do do this:

map.emplace(123, 321);

By the way, the reason I have my own implementation of pair is that std::pair is not trivially copyable because of it's float -> double conversion "feature". So for basic types robin_hood::pair should be faster.

jthemphill commented 4 years ago

Okay, I'll close this if this is intentional.

chipsethan commented 3 years ago

Still, I hope to support std::pair instead of robin_hood::pair for compatible reason.