Skip to content

Migrate from boost::unordered_map to std::unordered_map#3107

Merged
SergioRAgostinho merged 2 commits intoPointCloudLibrary:masterfrom
SergioRAgostinho:unordered_map
May 31, 2019
Merged

Migrate from boost::unordered_map to std::unordered_map#3107
SergioRAgostinho merged 2 commits intoPointCloudLibrary:masterfrom
SergioRAgostinho:unordered_map

Conversation

@SergioRAgostinho
Copy link
Copy Markdown
Member

No description provided.

@SergioRAgostinho SergioRAgostinho added changelog: ABI break Meta-information for changelog generation c++14 labels May 28, 2019
@SergioRAgostinho SergioRAgostinho added this to the pcl-1.10.0 milestone May 28, 2019
}
};
typedef boost::unordered_multimap<HashKeyStruct, std::pair<size_t, size_t> > FeatureHashMapType;
typedef std::unordered_multimap<HashKeyStruct, std::pair<size_t, size_t>, boost::hash<HashKeyStruct>> FeatureHashMapType;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The std library does not expose hashing specializations for containers. Since we're still using boost, I leveraged it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, does not seem to work and I don't have any other proposals. Maybe leave boost::unordered_multimap for now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't got back to this once I created the PR. I have one alternative in mind but I still need to look with special care to the error message.

Note that in cppreference even mentions boost::hash for hashing the std::pair container.

Note: additional specializations for std::pair and the standard container types, as well as utility functions to compose hashes are available in boost.hash
Source: https://en.cppreference.com/w/cpp/utility/hash

};

typedef boost::unordered_map<int, Leaf, boost::hash<int>, std::equal_to<int>, Eigen::aligned_allocator<int> > HashMap;
typedef std::unordered_map<int, Leaf, std::hash<int>, std::equal_to<int>, Eigen::aligned_allocator<std::pair<const int, Leaf>>> HashMap;
Copy link
Copy Markdown
Member Author

@SergioRAgostinho SergioRAgostinho May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allocator looked suspicious. The convention is to allocate the full object that will be created. The moment it started using std::unordered_map, it triggered a static assertion which complained it was not a valid allocator, so now it's changed to conform with the default.

@SergioRAgostinho SergioRAgostinho mentioned this pull request May 28, 2019
11 tasks
const std::size_t h2 = std::hash<int>{} (s.second.first);
const std::size_t h3 = std::hash<int>{} (s.second.second.first);
const std::size_t h4 = std::hash<int>{} (s.second.second.second);
return h1 ^ (h2 << 1) ^ (h3 << 2) ^ (h4 << 3);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we can cast HashKeyStruct to a std::pair<int, std::pair<int, std::pair<int, int>>> here and return the output of boost::hash.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but to me looks good as it is now.

@SergioRAgostinho SergioRAgostinho merged commit 112d870 into PointCloudLibrary:master May 31, 2019
@SergioRAgostinho SergioRAgostinho deleted the unordered_map branch May 31, 2019 12:02
@taketwo taketwo changed the title Migrate unordered_map from boost -> std Migrate from boost::unordered_map to std::unordered_map Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: ABI break Meta-information for changelog generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants