Using std::unordered_*

[David Benjamin]

So, we have base/containers/hash_tables.h. C++11 has std::unordered_*. We should switch to those.

With https://groups.google.com/a/chromium.org/d/msg/chromium-dev/cjiUwbUhQXM/wCn8D4A1c2UJ, base/containers/hash_tables.h’s semantics were largely aligned with std::unordered_*. Now we can use the real thing everywhere.

I did an initial CL and the try jobs pass. It’s not meant to land as-is, just to show that std::unordered_* works. (After you fix tons of IWYU violations...)

https://codereview.chromium.org/1502373009/

Some issues:

1) hash_tables.h has a base::string16 specialization. C++11 only provides it for std::string, std::wstring, etc., and not any std::basic_string instantiation, so a priori we’d get a hasher on Windows (string16 = wstring) and not other platforms (string16 = custom basic_string<T>). This is easy to resolve: move the std::hash specialization into string16.h[*]. Whether specializations are allowed in general (below), I think keeping base::string16 aligned across platforms is correct.

2) hash_tables.h defines a std::pair specialization and C++11 does not. We have a lot of hash tables which use a std::pair key. We could define it in some utility header in base or we could provide a base::PairHash<A, B> which consumers explicitly pass into std::unordered_*. I prefer the latter. I don’t like having std::unordered_set<std::pair<int, int>> change based on whether a third header, distinct from std::pair’s header or std::unordered_set’s header. is included.

3) The Google style guide prohibits specializations of std::hash altogether:

https://google.github.io/styleguide/cppguide.html#std_hash

We do it a lot:

https://codereview.chromium.org/1502373009/diff/100001/base/containers/hash_tables.h

The style guide talks about reducing to size_t too many times, but all our custom hashes already do that. I also don’t know much about hash algorithms, so I could be misunderstanding. I don’t have an opinion as to which way we go here. (If we decide to move to explicit custom hashes, I'm happy to do the conversion of current things.)

Thoughts?

David

[*] I toyed with using std::u16string instead, but some code around ICU breaks because char16_t and uint16_t aren’t the same. (Really?! How are you supposed to avoid strict aliasing problems with all existing APIs ever?) Since the fix is easy, I propose starting with the std::hash specialization and we adopt std::u16string later, if ever.

[Brett Wilson]

On Mon, Dec 21, 2015 at 3:20 PM, David Benjamin <davi...@chromium.org> wrote:

So, we have base/containers/hash_tables.h. C++11 has std::unordered_*. We should switch to those.

With https://groups.google.com/a/chromium.org/d/msg/chromium-dev/cjiUwbUhQXM/wCn8D4A1c2UJ, base/containers/hash_tables.h’s semantics were largely aligned with std::unordered_*. Now we can use the real thing everywhere.

I did an initial CL and the try jobs pass. It’s not meant to land as-is, just to show that std::unordered_* works. (After you fix tons of IWYU violations...)

https://codereview.chromium.org/1502373009/

Some issues:

1) hash_tables.h has a base::string16 specialization. C++11 only provides it for std::string, std::wstring, etc., and not any std::basic_string instantiation, so a priori we’d get a hasher on Windows (string16 = wstring) and not other platforms (string16 = custom basic_string<T>). This is easy to resolve: move the std::hash specialization into string16.h[*]. Whether specializations are allowed in general (below), I think keeping base::string16 aligned across platforms is correct.

2) hash_tables.h defines a std::pair specialization and C++11 does not. We have a lot of hash tables which use a std::pair key. We could define it in some utility header in base or we could provide a base::PairHash<A, B> which consumers explicitly pass into std::unordered_*. I prefer the latter. I don’t like having std::unordered_set<std::pair<int, int>> change based on whether a third header, distinct from std::pair’s header or std::unordered_set’s header. is included.

Explicitly passing a helper hash functions SGTM.

 

 

3) The Google style guide prohibits specializations of std::hash altogether:

https://google.github.io/styleguide/cppguide.html#std_hash

We do it a lot:

https://codereview.chromium.org/1502373009/diff/100001/base/containers/hash_tables.h

The style guide talks about reducing to size_t too many times, but all our custom hashes already do that. I also don’t know much about hash algorithms, so I could be misunderstanding. I don’t have an opinion as to which way we go here. (If we decide to move to explicit custom hashes, I'm happy to do the conversion of current things.

The Google style guide is a bit funny. It seems to be saying "don't write custom hash functions but if you do, use the old way to write custom hash functions until we figure out what to do". The "old way" seems to be to write a hash template specialization HASH_NAMESPACE::hash<T> which ends up working exactly the same as overriding std::hash but for a different container type. o_O

This is pretty unconvincing, especially given that we don't have these legacy ones (well, our legacy ones are so bad as to be irrelevant).

We should definitely provide an override for string16 in any case. I would also advocate for GURL having a default one since I think this is a fundamental type for us.

I've not used unordered set. I presume it's super easy to specify the use of a custom hash function. If this is correct, I don't see much disadvantage about just specifying it explicitly. It has some advantages of being explicit, and slightly discourages writing random hash functions which is often a bad idea.

Brett

[Daniel Cheng]

On Mon, Dec 21, 2015 at 3:20 PM David Benjamin <davi...@chromium.org> wrote:

So, we have base/containers/hash_tables.h. C++11 has std::unordered_*. We should switch to those.

Is there a compelling reason to differ from Google style here? It recommends using std::unordered_* for types that std::hash natively supports, and otherwise just falling back to the legacy hash containers or using a custom hasher.

 

With https://groups.google.com/a/chromium.org/d/msg/chromium-dev/cjiUwbUhQXM/wCn8D4A1c2UJ, base/containers/hash_tables.h’s semantics were largely aligned with std::unordered_*. Now we can use the real thing everywhere.

I did an initial CL and the try jobs pass. It’s not meant to land as-is, just to show that std::unordered_* works. (After you fix tons of IWYU violations...)

https://codereview.chromium.org/1502373009/

Some issues:

1) hash_tables.h has a base::string16 specialization. C++11 only provides it for std::string, std::wstring, etc., and not any std::basic_string instantiation, so a priori we’d get a hasher on Windows (string16 = wstring) and not other platforms (string16 = custom basic_string<T>). This is easy to resolve: move the std::hash specialization into string16.h[*]. Whether specializations are allowed in general (below), I think keeping base::string16 aligned across platforms is correct.

2) hash_tables.h defines a std::pair specialization and C++11 does not. We have a lot of hash tables which use a std::pair key. We could define it in some utility header in base or we could provide a base::PairHash<A, B> which consumers explicitly pass into std::unordered_*. I prefer the latter. I don’t like having std::unordered_set<std::pair<int, int>> change based on whether a third header, distinct from std::pair’s header or std::unordered_set’s header. is included.

The style guide is pretty explicit about this:

    Due to exactly that issue, std::hash does not work with std::pair

    or std::tuple, and the language does not allow us to extend it to

    support them.

I think this is due to the fact that you're only allowed to specialize std::hash on user-defined types. Doing otherwise is undefined behavior.

 

3) The Google style guide prohibits specializations of std::hash altogether:

https://google.github.io/styleguide/cppguide.html#std_hash

We do it a lot:

https://codereview.chromium.org/1502373009/diff/100001/base/containers/hash_tables.h

The style guide talks about reducing to size_t too many times, but all our custom hashes already do that. I also don’t know much about hash algorithms, so I could be misunderstanding. I don’t have an opinion as to which way we go here. (If we decide to move to explicit custom hashes, I'm happy to do the conversion of current things.)

http://go/enabling-unordered (internal-only =(, sorry...) is worth reading. 

Daniel

 

Thoughts?

David

[*] I toyed with using std::u16string instead, but some code around ICU breaks because char16_t and uint16_t aren’t the same. (Really?! How are you supposed to avoid strict aliasing problems with all existing APIs ever?) Since the fix is easy, I propose starting with the std::hash specialization and we adopt std::u16string later, if ever.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF8qwaDsfsCoOb-%2Bk8ZMfJMjE1o0YcwNQFwbCO0KUg%3Df0myCCA%40mail.gmail.com.

[David Benjamin]

On Mon, Dec 21, 2015 at 6:55 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Dec 21, 2015 at 3:20 PM David Benjamin <davi...@chromium.org> wrote:

So, we have base/containers/hash_tables.h. C++11 has std::unordered_*. We should switch to those.

Is there a compelling reason to differ from Google style here? It recommends using std::unordered_* for types that std::hash natively supports, and otherwise just falling back to the legacy hash containers or using a custom hasher.

The legacy containers are compiler-specific which matters for us, but doesn't matter for a lot of Google code. MSVC's legacy container, notably, is completely different from __gnu_cxx's and doesn't even accept operator== on the key. It wants operator<. See thread below where I switched it to std::unordered_* early. (Although I didn't realize specializing std::pair was undefined behavior. I should have made it a template alias like the GCC codepath. Oops.)

I think it's worth finishing that transition so as to cut down on compiler-specific code (all to workaround compiler-specific behavior) now that we have an option with standardized behavior. If not use std::unordered_* straight, we should at least implement base::hash_* using it (template alias with different default hash?). go/enabling-unordered rejected that option, but perhaps the tradeoffs change because a combination of std::unordered_* and legacy containers is much worse for us.

[Brett Wilson]

On Mon, Dec 21, 2015 at 3:55 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Dec 21, 2015 at 3:20 PM David Benjamin <davi...@chromium.org> wrote:

So, we have base/containers/hash_tables.h. C++11 has std::unordered_*. We should switch to those.

Is there a compelling reason to differ from Google style here? It recommends using std::unordered_* for types that std::hash natively supports, and otherwise just falling back to the legacy hash containers or using a custom hasher.

Chrome doesn't have the legacy hash functions that the guide refers to. We used the weird transitional platform-specific ones which are pretty confusing and we should get rid of them ASAP.

Brett

[Dana Jansens]

On Mon, Dec 21, 2015 at 3:20 PM, David Benjamin <davi...@chromium.org> wrote:

So, we have base/containers/hash_tables.h. C++11 has std::unordered_*. We should switch to those.

Blink also has a hash table implementation, which to my understanding (citation required) performs much better than std::unordered_map. I'd like us to converge between chromium and blink more than I'd like us to use whatever google3 does in chromium, and something else in blink.

I've avoided pushing us toward unordered_* because I'd like someone to do the work of figuring out if the blink Hash* classes are better and provide data for that and move us all to sharing them if possible.

Also, we would need to use better hash functions than std::hash with such a hash table, as it (AIUI) uses power-of-2 table sizes instead of primes, which is much better for performance (especially on ARM which has very slow integer division/mod) but requires the hash function to evenly distribute its outputs. So for instance, identity hash functions for integers can be problematic if the input set is not contiguous.

Here's a related bug: https://code.google.com/p/chromium/issues/detail?id=554292

 

--

[Brett Wilson]

On Mon, Jan 4, 2016 at 2:20 PM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Dec 21, 2015 at 3:20 PM, David Benjamin <davi...@chromium.org> wrote:

So, we have base/containers/hash_tables.h. C++11 has std::unordered_*. We should switch to those.

Blink also has a hash table implementation, which to my understanding (citation required) performs much better than std::unordered_map. I'd like us to converge between chromium and blink more than I'd like us to use whatever google3 does in chromium, and something else in blink.

I've avoided pushing us toward unordered_* because I'd like someone to do the work of figuring out if the blink Hash* classes are better and provide data for that and move us all to sharing them if possible.

Also, we would need to use better hash functions than std::hash with such a hash table, as it (AIUI) uses power-of-2 table sizes instead of primes, which is much better for performance (especially on ARM which has very slow integer division/mod) but requires the hash function to evenly distribute its outputs. So for instance, identity hash functions for integers can be problematic if the input set is not contiguous.

In my use of Blink's hash tables, I found them exceptionally user-unfriendly. I spent an entire afternoon just making my code not crash mysteriously. Blink's hash tables provided a bunch of optimizations for delete and uninitialized types that allows it to be more efficient than the regular hash tables, but at a usability cost. (It looks like the code has improved so I'm willing to revisit if others thing I'm wrong.)

I think the STL code is the way it is because that's what you end up with optimizing for "not wrong" usage. For the majority of Chrome code, we should also be optimizing for "not wrong." This does mean that certain specialized workloads may want something else more elaborate and we end up with two ways to do something. Or maybe we can come up with something magical that allows for both.

Brett

[Ryan Hamilton]

On Mon, Jan 4, 2016 at 3:46 PM, Brett Wilson <bre...@chromium.org> wrote:

I think the STL code is the way it is because that's what you end up with optimizing for "not wrong" usage. For the majority of Chrome code, we should also be optimizing for "not wrong." This does mean that certain specialized workloads may want something else more elaborate and we end up with two ways to do something. Or maybe we can come up with something magical that allows for both.

​Supporting STL would also make life easier for QUIC and SPDY which share code with an internal repository.​ We can, of course, make it work either way but having the ability to use STL would be convenient.

​Cheers,

Ryan

​

[Nico Weber]

I'm  with Brett that using the unsurprising class (unordered_map) seems best in general. unordered_map is a bit funny in that you'd want to use it over map<> mostly if you care about performance, but in that case you might want to use a hash map that's more efficient than unordered_map too. So unordered_map is for when you care about performance, but only a little -- or do document that you don't care about iteration order.

unordered_map seems strictly better than what have today (which in practice is unordered_map under a different name). So I think we should allow this (with explicit hashers in general, but a default hasher for string16) -- it doesn't change much in practice except remove a layer of indirection.

It might still be valuable to make the blink hash map usable outside of blink for very large hash maps (even though there's some risk of cargo-culted uses of it then), but that seems like it can be discussed later.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABiGVV-7ezZp%3D-hoHr7r60TLoeQkK4-%2BRLGuKx%2B387K6nGJwBQ%40mail.gmail.com.

[Nico Weber]

styleguide/OWNERS discussed this thread today. We should allow unordered_* for new code, and implement hash_tables.h on top of them. Use explicit hashers where possible, but provide a string16 hasher specialization (and maybe GURL, depends a bit on how very often this is used in practice). davidben@, do you think you can get your patch into landable shape?

[Łukasz Anforowicz]

<+nick@ and dcheng@>

On Thu, Jan 7, 2016 at 1:52 PM, Nico Weber <tha...@chromium.org> wrote:

styleguide/OWNERS discussed this thread today. We should allow unordered_* for new code, and implement hash_tables.h on top of them. Use explicit hashers where possible, but provide a string16 hasher specialization (and maybe GURL, depends a bit on how very often this is used in practice). davidben@, do you think you can get your patch into landable shape?

Any opinion on what is in currently in https://codereview.chromium.org/1529363006/?  In particular, please shout if you think that I shouldn't be landing a CL with a new specialization of BASE_HASH_NAMESPACE::hash<...>.

The current state of IdType<...> in https://codereview.chromium.org/1529363006/:

• BASE_HASH_NAMESPACE::hash<IdType<...>> is provided to support base::hash_map.  This means std::hash is provided on Windows (because on Windows we define BASE_HASH_NAMESPACE to expand into "std").
• No unordered_support is provided at this point (although it was present in earlier patchsets - thanks to dcheng@ for pointing out the undesirability of this)
  

In particular, I am not sure how the long-term future looks like given that:

• Chromium does use maps from id into objects and we use base::hash_map for some of them (i.e. in content/browser/downloads/save_package.h) and will probably want to use std::unordered_map for them in the future.
• IdType<...> doesn't make sense if it cannot represent save_item_id or other ids that currently are used as keys in base::hash_map
• I believe the standard library will provide the default hashers for int32_t, int64_t, etc and therefore one could argue that providing a default hasher for IdType<...> doesn't make the situation any worse (after all hasher for IdType<...> simply forwards to the hasher of the wrapped/underlying type).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiHnuZZT5qZaYOHZpakZ6TjZiNgRVC%3De%2BfO1EKtTtdDu1g%40mail.gmail.com.

--

Thanks,

Lukasz

[David Benjamin]

On Thu, Jan 7, 2016 at 1:52 PM Nico Weber <tha...@chromium.org> wrote:

styleguide/OWNERS discussed this thread today. We should allow unordered_* for new code, and implement hash_tables.h on top of them. Use explicit hashers where possible, but provide a string16 hasher specialization (and maybe GURL, depends a bit on how very often this is used in practice). davidben@, do you think you can get your patch into landable shape?

Sure. I'm at Real World Crypto this week, but I can toy with this when I'm back (and after I dig myself out of the email pit from being at a conference three days :-) ).

I assume by "allow unordered_* for new code", you mean "allow and prefer unordered_* for new code" with hash_tables.h implemented just for transition?

David

[Nico Weber]

On Fri, Jan 8, 2016 at 12:50 AM, David Benjamin <davi...@chromium.org> wrote:

On Thu, Jan 7, 2016 at 1:52 PM Nico Weber <tha...@chromium.org> wrote:

styleguide/OWNERS discussed this thread today. We should allow unordered_* for new code, and implement hash_tables.h on top of them. Use explicit hashers where possible, but provide a string16 hasher specialization (and maybe GURL, depends a bit on how very often this is used in practice). davidben@, do you think you can get your patch into landable shape?

Sure. I'm at Real World Crypto this week, but I can toy with this when I'm back (and after I dig myself out of the email pit from being at a conference three days :-) ).

I assume by "allow unordered_* for new code", you mean "allow and prefer unordered_* for new code" with hash_tables.h implemented just for transition?

Yes.

[Jeffrey Yasskin]

On Fri, Jan 8, 2016 at 7:11 AM, Nico Weber <tha...@chromium.org> wrote:
> On Fri, Jan 8, 2016 at 12:50 AM, David Benjamin <davi...@chromium.org>
> wrote:
>>
>> On Thu, Jan 7, 2016 at 1:52 PM Nico Weber <tha...@chromium.org> wrote:
>>>
>>> styleguide/OWNERS discussed this thread today. We should allow
>>> unordered_* for new code, and implement hash_tables.h on top of them. Use
>>> explicit hashers where possible, but provide a string16 hasher
>>> specialization (and maybe GURL, depends a bit on how very often this is used
>>> in practice). davidben@, do you think you can get your patch into landable
>>> shape?
>>
>>
>> Sure. I'm at Real World Crypto this week, but I can toy with this when I'm
>> back (and after I dig myself out of the email pit from being at a conference
>> three days :-) ).
>>
>> I assume by "allow unordered_* for new code", you mean "allow and prefer
>> unordered_* for new code" with hash_tables.h implemented just for
>> transition?
>
>
> Yes.

I'm reviewing a patch that introduces a
base::ScopedPtrHashMap<std::string, scoped_ptr<T>>. Should I suggest
std::unordered_map<std::string, scoped_ptr<T>> now, or should I wait
until David's patch lands?

Jeffrey

[Nico Weber]

I'd wait; we already have close to a hundred ScopedPtrHashMap clients. One more won't change things.

[David Benjamin]

Alright, my CL should be ready, try jobs willing. I'll send that once I've finished the follow-up to convert everything in //cc. I want to have one directory in the pipeline just as a trial run to make sure things work as expected. (Picked cc at random since it had a wide range of interesting base::hash_map and base::ScopedPtrHashMap uses.) Some things that came up:

Going directory-by-directory may be tricky for the widely-used things like base::FilePath's hash.

For sanity, we want to convert base::hash_map and base::ScopedPtrHashMap simultaneously when custom types are involved, unless we go add a third parameter to ScopedPtrHashMap or types define both custom hashers and BASE_HASH_NAMESPACE simultaneously (which would also alleviate the base::FilePath issue so it might make sense in some cases).

I decided to separate the base::hash_map default hash from std::hash (it just forwards to the default one with some additions) so that legacy BASE_HASH_NAMESPACE uses don't interfere with the new world. Also this way we don't specialize std::pair on std::hash.

Our Linux sysroot uses libstdc++ 4.6.3, so we're sensitive to a bug. If you use std::hash<A> where that isn't defined, you *link* error, not a compile error. libc++ and libstdc++ 4.8 don't have this bug. This is these two GCC bugs:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51558

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52931

libstdc++ 4.6.3 declares the template as:

  /// Primary class template hash.

  template<typename _Tp>

    struct hash : public __hash_base<size_t, _Tp>

    {

      size_t

      operator()(_Tp __val) const;

    };

Glancing at backward/hash_fun.h, the legacy __gnu_cxx::hash does not have this problem.

I can't think of cases where this blows up beyond unhelpful errors, but others here might. (I did try anonymous namespaces which is still a link-time error, but the same struct defined in another compilation unit doesn't resolve against the undefined symbol, so it seems good. Plus we wouldn't be doing that anyway per the style guide.)

David

[joh...@google.com]

Now that std::unordered_* is being allowed, can we use std::unordered_multimap::emplace?

It's much neater to be able to do:

my_multimap.emplace("key", "value");

rather than:

my_multimap.insert(std::make_pair<std::string, std::string>("key", "value"));

(even though they're similarly efficient, since the latter case performs move insertion)

I just wanted to check as I saw concerns about libstdc++4.6's support for std::map::emplace.

Thanks,

John

[Jeremy Roman]

On Fri, Jan 15, 2016 at 8:43 AM, <joh...@google.com> wrote:

Now that std::unordered_* is being allowed, can we use std::unordered_multimap::emplace?

I don't see why not.

 

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/44b52a7a-d864-4cc2-880c-02f7fbe421a2%40chromium.org.

[Daniel Cheng]

What about std::piecewise_construct and std::forward_as_tuple, which are required to emplace types with multi-arg constructors?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13edVncjMC8iWx4Sc8t%2BYpzQNf%3De3iyy9F3Q0ErRxmW7jQ%40mail.gmail.com.

[Jeremy Roman]

I'd consider that a separate discussion because it involves std::tuple which isn't presently allowed. unordered_map::emplace is still useful for the common case.

(Personally I'm not sure I like the aesthetics of std::piecewise_construct+std::forward_as_tuple, so I'd probably prefer using emplace with movable rvalues, but I haven't given it a great deal of thought. Not sure what others think.)

[Jeffrey Yasskin]

My 2¢: Don't be fancy. map.emplace(key, value) is more concise and a bit easier to read than map.insert(make_pair(key, value)), and map.emplace(KeyType(arg1, arg2), ValueType(arg3, arg4)) is also more concise and much easier to read than map.emplace(std::piecewise_construct, forward_as_tuple(arg1, arg2), forward_as_tuple(arg3, arg4)). If you have a benchmark showing that avoiding the move-construction is important, then you have to decide between making the move constructor more efficient and obfuscating the map code, but without that benchmark, just do the simple thing.

[Daniel Cheng]

On Fri, Jan 15, 2016 at 10:55 AM Jeffrey Yasskin <jyas...@chromium.org> wrote:

My 2¢: Don't be fancy. map.emplace(key, value) is more concise and a bit easier to read than map.insert(make_pair(key, value)), and map.emplace(KeyType(arg1, arg2), ValueType(arg3, arg4)) is also more concise and much easier to read than map.emplace(std::piecewise_construct, forward_as_tuple(arg1, arg2), forward_as_tuple(arg3, arg4)). If you have a benchmark showing that avoiding the move-construction is important, then you have to decide between making the move constructor more efficient and obfuscating the map code, but without that benchmark, just do the simple thing.

Using the move-construction with emplace is equivalent to passing rvalues to insert, right?

[Jeremy Roman]

On Fri, Jan 15, 2016 at 2:04 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Fri, Jan 15, 2016 at 10:55 AM Jeffrey Yasskin <jyas...@chromium.org> wrote:

My 2¢: Don't be fancy. map.emplace(key, value) is more concise and a bit easier to read than map.insert(make_pair(key, value)), and map.emplace(KeyType(arg1, arg2), ValueType(arg3, arg4)) is also more concise and much easier to read than map.emplace(std::piecewise_construct, forward_as_tuple(arg1, arg2), forward_as_tuple(arg3, arg4)). If you have a benchmark showing that avoiding the move-construction is important, then you have to decide between making the move constructor more efficient and obfuscating the map code, but without that benchmark, just do the simple thing.

Using the move-construction with emplace is equivalent to passing rvalues to insert, right?

Readability is the thing to me.

It's the difference between

map.emplace("hello", ComplexValue(1, 2, 3));

and

map.insert(std::make_pair<std::string, ComplexValue>("hello", ComplexValue(1, 2, 3)));

To me, piecewise_construct is usually harder to read, so you're only (potentially) winning on a little efficiency:

map.emplace(std::piecewise_construct, std::forward_as_tuple("hello"), std::forward_as_tuple(1, 2, 3));

(In many cases you could write 'map["hello"] = ComplexValue(1, 2, 3)' as well, though that's not always equivalent.)

[Jeffrey Yasskin]

On Fri, Jan 15, 2016 at 11:13 AM, Jeremy Roman <jbr...@chromium.org> wrote:

map.insert(std::make_pair<std::string, ComplexValue>("hello", ComplexValue(1, 2, 3)));

Unrelated, but don't pass template arguments to make_pair(). Either use std::pair<std::string, ComplexValue>(...) or just std::make_pair(...). Passing template arguments to make_pair causes compiler errors when the value arguments aren't rvalues.

[Jeffrey Yasskin]

On Fri, Jan 15, 2016 at 11:04 AM, Daniel Cheng <dch...@chromium.org> wrote:

On Fri, Jan 15, 2016 at 10:55 AM Jeffrey Yasskin <jyas...@chromium.org> wrote:

My 2¢: Don't be fancy. map.emplace(key, value) is more concise and a bit easier to read than map.insert(make_pair(key, value)), and map.emplace(KeyType(arg1, arg2), ValueType(arg3, arg4)) is also more concise and much easier to read than map.emplace(std::piecewise_construct, forward_as_tuple(arg1, arg2), forward_as_tuple(arg3, arg4)). If you have a benchmark showing that avoiding the move-construction is important, then you have to decide between making the move constructor more efficient and obfuscating the map code, but without that benchmark, just do the simple thing.

Using the move-construction with emplace is equivalent to passing rvalues to insert, right?

Not exactly equivalent, but close enough.

[Jeremy Roman]

Errm, right. I saw that in johnme's example and didn't think about it. Another reason to like unordered_map::emplace, perhaps.

[Jeremy Roman]

Oh, and as one more thought on that: there is internal guidance at go/totw:67 which aligns with what I (and, if I understood correctly, others) have said here.

[Nico Weber]

On Fri, Jan 15, 2016 at 10:36 AM, Jeremy Roman <jbr...@chromium.org> wrote:

On Fri, Jan 15, 2016 at 8:43 AM, <joh...@google.com> wrote:

Now that std::unordered_* is being allowed, can we use std::unordered_multimap::emplace?

I don't see why not.

Well, if it doesn't build on the bots, then we can't use it :-) Did someone try that? I think Linux's libstdc++4.6 might've been missing emplace() for at least some container classes.

Other than that, I agree with what Jeffrey and Jeremy said. I just stress that If you want insert-or-update, or if you know the object isn't in the map yet, use operator[]. That's often an option.

[David Benjamin]

Sorry, I forgot to circle back on this thread after the change stuck.

The change stuck.

https://codereview.chromium.org/1502373009

I've assigned both bugs to convert base::hash_map and base::ScopedPtrHashMap to myself and will probably whittle away at it when I have cycles, though do feel free to whittle away at it in your own directories. Note that BASE_HASH_NAMESPACE and ScopedPtrHashMap conversions are not quite mechanical. ScopedPtrHashMap has a different API from std::unordered_map of scoped_ptrs.
https://code.google.com/p/chromium/issues/detail?id=576864
https://code.google.com/p/chromium/issues/detail?id=579229

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiFF%2BRB35xN3O5jxYMw_LeSyGEjgSR9oqBbQwK1Xim7qSQ%40mail.gmail.com.