dense_hash_{map,set}: C++11 support: move, emplace*, cbegin/cend, ...

[david-grs]

Hey guys,

I was working the last few days on adding C++11 support to dense_hash_map/set. I added a lot of unit tests to test the new members and be sure there was no copy when you move keys or values.

Let me know what you think!

Cheers
David

[PhilipDeegan]

Hey Dave, thanks.

I've tried building the tests on windows and found the following on MSVC15U3
http://pastebin.com/raw/puptK2gt

Similarly for MSVC15U1, the error might be a little more informative though.
http://pastebin.com/raw/9cMHi7mr

[david-grs]

@dekken thanks, I was testing with gcc 6 and clang 3.8. Actually, it was not building with gcc 5 for the same reason. I installed VS express 2015 to be sure my commit fixed the issue.

[graehl]

nice. thanks.

[david-grs]

@dekken did you have a chance to look at it :)?

[PhilipDeegan]

Sorry no, shall tomorrow/Saturday.

I did confirm that tests are running.
I have also tested with the current version of google test https://github.com/google/googletest, which has problems but that's a different story.

[david-grs]

cool, no worries! let me know if I can do anything to help you or if you have any concerns/comments regarding the code.

[donovanhide]

Firstly, thanks for working on this, and apologies for not responding earlier. As the person responsible for originally trying to make this codebase a bit more "C++11-like" I'm grateful for the help!

I'm not a regular user of C++ these days, using Go much more often, so as a result my knowledge of move semantics is limited and I'm not best equipped to review this code. Main points I'd raise are:

• Would this same code be easily transferrable to sparsehash map and set? Would some refactoring of common code make that easier?
• Do the code changes actually improve performance?
• Would all existing client code still work after the changes?
• How should we document new features going forwards? We need to host the docs pages, perhaps using gh-pages or a wiki.

@david-grs if you are keen to get more involved in this project in the long term, happy to give you access rights.

[david-grs]

Yes, I already thought/planned about adding it to sparsehash map/set. I simply didn't want to send you a PR that would changed all the files of the library, it is already big enough :) It is better step by step imho.

The code improves de facto performance on insertion, as it moves the key and/or value when calling insert() / operator[], or constructs in place when calling emplace (in this case, no copy or move at all). Some of the unit tests I added verify that the number of copies / moves done for each operation is the one expected when moving / constructing in place objects. If you comment one of the new overloading like insert(value_type&&) for example, you will immediately see that the unit test will call insert(const value_type&) and it will fail by indicating a call of the copy ctor.

But this is more than performance ; it is about keeping the API as close as possible to STL map / unordered_map, in order to allow templated code. That was my main motivation to be honest ; having a specialization in my templates for dense_hash_map because it does not have emplace is really annoying :)

There is no breaking change in the API. These changes are the same that the ones on std::map since C++11, and everything has to be backward compatible. This is why all these changes in dense_hash_{map,set} are also backward compatible. They contain new overloadings (insert / operator[] with move support), new method (emplace, emplace_hint) or signature changes (but with backward compatibility), for example void erase(iterator it) changed to iterator erase(const_iterator it), which is fine as const_iterator can be constructed from an iterator implicitly.

About the documentation, gh-pages sounds great!

@donovanhide if you're confortable with that and you prefer than regular pull requests, that's fine with me too. As I said, I plan to add it to sparsehash too. C++17 is also almost here :)...

[donovanhide]

Thanks for the answers! Let's solve #6 first and then add some docs to this PR. And then consider the sparsehash map/set as another PR.

[donovanhide]

@david-grs if you can add some docs for your changes to this PR now that they are in the docs folder, I'm sure we can merge!

[david-grs]

I am looking at it now. I see that you link the defined methods to SGI concepts (e.g. insert(const value_type&) is linked to Unique Associative Container), which looks nice, but the problem is that SGI STL is no longer maintained anymore... So all the new methods are not present in the SGI concepts ; nothing about rvalue references or in place construction (emplace), etc.

What do you think? I think the best would be to link to cppreference, for example insert(const value_type&) should be linked to Unordered Associative Container. Then, another step would be to change all the SGI links to cppreference, to have something consistent and up-to-date.

[david-grs]

@donovanhide Done. Finally, I linked to SGI for now, to be consistent. Let's rename / change the links in another PR.

[donovanhide]

Thanks for doing that! We should definitely update links to point to cppreference.com if what we have does fully match the UnorderedAssociativeContainer concept.