Allow std::unordered_*.

Allow std::unordered_*.

base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.

Also fix a host of IWYU problems that arose from this CL.

(NOPRESUBMIT because the wstring presubmit check is overzealous
and complains about the reference to wstring in the comment.)

NOPRESUBMIT=true
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3f37f7f1459e7b5a452c0e433493e0a6e9649ca7
Cr-Commit-Position: refs/heads/master@{#370553}

[davidben, 2015-12-09 20:11:27]

Description was changed from

==========
Test CL to make sure unordered_* works.

BUG=
==========

to

==========
Test CL to make sure unordered_* works.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[brucedawson, 2015-12-10 01:21:22]

The win_chromium_rel_ng try job
(https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...)
failed due to https://code.google.com/p/chromium/issues/detail?id=567377 - two
hour job timeout

[davidben, 2016-01-14 20:26:48]

Description was changed from

==========
Test CL to make sure unordered_* works.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.
    
BUG=576864
==========

[davidben, 2016-01-14 20:37:06]

Description was changed from

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.
    
BUG=576864
==========

to

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.
    
BUG=576864
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[davidben, 2016-01-14 20:40:41]

davidben@chromium.org changed reviewers:
+ danakj@chromium.org, thakis@chromium.org

[davidben, 2016-01-14 20:40:41]

This is going to need no end of OWNERS stamps thanks to all the IWYU problems it
surfaces, but I'll start with you two as the main reviewers.

This CL doesn't convert anything to std::unordered_* yet just because it's
pretty big as it is. To get a preview of the new world order,
https://codereview.chromium.org/1587283002/ converts the cc/ directory over.

[danakj, 2016-01-14 21:29:10]

https://codereview.chromium.org/1502373009/diff/200001/base/containers/hash_t...
File base/containers/hash_tables.h (right):

https://codereview.chromium.org/1502373009/diff/200001/base/containers/hash_t...
base/containers/hash_tables.h:36: return base::HashPair(value.first,
value.second);
Can make this use PairHash? It will help with peoples' copypasta

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
File base/containers/pair_hash.h (right):

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
base/containers/pair_hash.h:77: inline std::size_t HashPair(T1 value1, T2
value2) {
These helpers can move into an internal namespace?

I'm a bit :/ that this file defines a PairHash and a HashPair.. maybe
internal::HashPair would be enough to differenetiate?

https://codereview.chromium.org/1502373009/diff/200001/base/strings/string16.h
File base/strings/string16.h (right):

https://codereview.chromium.org/1502373009/diff/200001/base/strings/string16....
base/strings/string16.h:189: // platforms, where base::string16 is a typedef for
std::wstring.
s/typedef/type alias/

https://codereview.chromium.org/1502373009/diff/200001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (right):

https://codereview.chromium.org/1502373009/diff/200001/styleguide/c++/c++11.h...
styleguide/c++/c++11.html:433: <td>Per the <a
href="https://google.github.io/styleguide/cppguide.html#std_hash">Google style
guide</a>, specify custom hashers instead of specializing <code>std::hash</code>
for custom types. Existing uses of <code>base::hash_map</code>, etc., will be <a
href="https://crbug.com/576864">replaced throughout the codebase</a>. <a
href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/nCdjQqnouO4">Discussion
thread</a>.</td>
"Existing uses of <code>base::hash_map</code>, e
      tc., will be <a href="https://crbug.com/576864">replaced throughout the
codebase
      </a>."

^ That part is going to bitrot, I would leave it out.

[davidben, 2016-01-14 21:34:34]

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
File base/containers/pair_hash.h (right):

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
base/containers/pair_hash.h:77: inline std::size_t HashPair(T1 value1, T2
value2) {
On 2016/01/14 21:29:10, danakj wrote:
> These helpers can move into an internal namespace?
> 
> I'm a bit :/ that this file defines a PairHash and a HashPair.. maybe
> internal::HashPair would be enough to differenetiate?

They actually do get called directly for types that are pair-like. Though I
guess one could make a temporary pair or something? I was even debating whether
we should standardize on HashPair instead and ditch PairHash because PairHash is
not actually very general-purpose...

I didn't notice this until doing this, but PairHash and HashPair don't work for
arbitrary Type1 and Type2. They have to be numeric types, so it's not even that
good of a name. I left it PairHash because HairPair is also generic, but meant
to ask in review and forgot. :-(

Maybe it should be something like:
HashPair -> HashInts
PairHash -> IntPairHash
pair_hash.h -> ???

[danakj, 2016-01-14 21:47:12]

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
File base/containers/pair_hash.h (right):

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
base/containers/pair_hash.h:77: inline std::size_t HashPair(T1 value1, T2
value2) {
On 2016/01/14 21:34:34, davidben wrote:
> On 2016/01/14 21:29:10, danakj wrote:
> > These helpers can move into an internal namespace?
> > 
> > I'm a bit :/ that this file defines a PairHash and a HashPair.. maybe
> > internal::HashPair would be enough to differenetiate?
> 
> They actually do get called directly for types that are pair-like. Though I
> guess one could make a temporary pair or something?

Oh.. pair-like.. ya. We probably don't want to force hash lookups to construct
pairs.

> I was even debating whether
> we should standardize on HashPair instead and ditch PairHash because PairHash
is
> not actually very general-purpose...

That seems like it could be reasonable, as it's very clean now to implement your
own hasher. Are there very many more hash keys of std::pair<> than of other
types?

> I didn't notice this until doing this, but PairHash and HashPair don't work
for
> arbitrary Type1 and Type2. They have to be numeric types, so it's not even
that
> good of a name. I left it PairHash because HairPair is also generic, but meant
> to ask in review and forgot. :-(
> 
> Maybe it should be something like:
> HashPair -> HashInts

sgtm

> PairHash -> IntPairHash

sgtm

> pair_hash.h -> ???

How about just hash.h. If we add more functions for other standard composite
types, they can probably all go in here. And then that feels like it makes
HashInts a bit more discoverable/belonging/

[danakj, 2016-01-14 21:48:06]

On 2016/01/14 20:40:41, davidben wrote:
> This is going to need no end of OWNERS stamps thanks to all the IWYU problems
it
> surfaces, but I'll start with you two as the main reviewers.

I would recommend TBRing owners for IWYU-only changes btw.

[davidben, 2016-01-15 00:25:42]

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
File base/containers/pair_hash.h (right):

https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
base/containers/pair_hash.h:77: inline std::size_t HashPair(T1 value1, T2
value2) {
On 2016/01/14 21:47:12, danakj wrote:
> > I was even debating whether
> > we should standardize on HashPair instead and ditch PairHash because
PairHash
> is
> > not actually very general-purpose...
> 
> That seems like it could be reasonable, as it's very clean now to implement
your
> own hasher. Are there very many more hash keys of std::pair<> than of other
> types?

I'm not sure. I'll give it a try and see how commonly that specialization was
used. Intuitively I would think this wouldn't be common, but maybe we have lots
of hash tables on pair<child_id, route_id> or something.


> > I didn't notice this until doing this, but PairHash and HashPair don't work
> for
> > arbitrary Type1 and Type2. They have to be numeric types, so it's not even
> that
> > good of a name. I left it PairHash because HairPair is also generic, but
meant
> > to ask in review and forgot. :-(
> > 
> > Maybe it should be something like:
> > HashPair -> HashInts
> 
> sgtm
> 
> > PairHash -> IntPairHash
> 
> sgtm
> 
> > pair_hash.h -> ???
> 
> How about just hash.h. If we add more functions for other standard composite
> types, they can probably all go in here. And then that feels like it makes
> HashInts a bit more discoverable/belonging/

So, there's already a base/hash.h, so we probably shouldn't have a
base/containers/hash.h. But in that case, perhaps HashInts* and IntPairHash (if
not removed) should just go in base/hash.h? Though then that raises more
silliness like base::Hash(std::string) uses base::SuperFastHash which is not the
hash algorithm that StringPiece uses. And it hashes to uint32_t instead of
size_t.

[danakj, 2016-01-15 00:35:21]

On Thu, Jan 14, 2016 at 4:25 PM, <davidben@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
> File base/containers/pair_hash.h (right):
>
>
>
https://codereview.chromium.org/1502373009/diff/200001/base/containers/pair_h...
> base/containers/pair_hash.h:77: inline std::size_t HashPair(T1 value1,
> T2 value2) {
> On 2016/01/14 21:47:12, danakj wrote:
>
>> > I was even debating whether
>> > we should standardize on HashPair instead and ditch PairHash because
>>
> PairHash
>
>> is
>> > not actually very general-purpose...
>>
>
> That seems like it could be reasonable, as it's very clean now to
>>
> implement your
>
>> own hasher. Are there very many more hash keys of std::pair<> than of
>>
> other
>
>> types?
>>
>
> I'm not sure. I'll give it a try and see how commonly that
> specialization was used. Intuitively I would think this wouldn't be
> common, but maybe we have lots of hash tables on pair<child_id,
> route_id> or something.
>
>
> > I didn't notice this until doing this, but PairHash and HashPair
>>
> don't work
>
>> for
>> > arbitrary Type1 and Type2. They have to be numeric types, so it's
>>
> not even
>
>> that
>> > good of a name. I left it PairHash because HairPair is also generic,
>>
> but meant
>
>> > to ask in review and forgot. :-(
>> >
>> > Maybe it should be something like:
>> > HashPair -> HashInts
>>
>
> sgtm
>>
>
> > PairHash -> IntPairHash
>>
>
> sgtm
>>
>
> > pair_hash.h -> ???
>>
>
> How about just hash.h. If we add more functions for other standard
>>
> composite
>
>> types, they can probably all go in here. And then that feels like it
>>
> makes
>
>> HashInts a bit more discoverable/belonging/
>>
>
> So, there's already a base/hash.h, so we probably shouldn't have a
> base/containers/hash.h. But in that case, perhaps HashInts* and
> IntPairHash (if not removed) should just go in base/hash.h? Though then
> that raises more silliness like base::Hash(std::string) uses
> base::SuperFastHash which is not the hash algorithm that StringPiece
> uses. And it hashes to uint32_t instead of size_t.
>

Oh, heh, ya let's put them in that file.

SuperFastHash is also kinda bad, I knew we used it somewhere but didn't
realize where, thanks for pointing it out :) Should be base::HashString not
base::Hash too probably. Definitely some cleanup to do here..

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

[davidben, 2016-01-16 00:22:23]

Moved stuff to base/hash.h and renamed as discussed.

I haven't gotten rid of the old std::pair specialization or IntPairHash. It does
seem like there's a lot of them, but I'm not sure since I had to just tackle
compile failures one-by-one and got to a ScopedPtrHashMap which would have
required a custom hasher.

I did, as a result of that exploration, switch IntPairHash to a single-parameter
template (see below), but TBH I still find it kind of strange. It's hard to grep
for uses of integer pairs since most call sites set up a typedef first. For
example:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...


What do you two think? Shall I keep IntPairHash in there and, in later CLs, move
call sites that like that one to it, or get rid of it (keeping the
BASE_HASH_NAMESPACE::hash std::pair specialization around) and say callers like
that should make a little struct and whatnot when migrating to
std::unordered_map.

https://codereview.chromium.org/1502373009/diff/200001/base/strings/string16.h
File base/strings/string16.h (right):

https://codereview.chromium.org/1502373009/diff/200001/base/strings/string16....
base/strings/string16.h:189: // platforms, where base::string16 is a typedef for
std::wstring.
On 2016/01/14 21:29:10, danakj wrote:
> s/typedef/type alias/

Done.

https://codereview.chromium.org/1502373009/diff/200001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (right):

https://codereview.chromium.org/1502373009/diff/200001/styleguide/c++/c++11.h...
styleguide/c++/c++11.html:433: <td>Per the <a
href="https://google.github.io/styleguide/cppguide.html#std_hash">Google style
guide</a>, specify custom hashers instead of specializing <code>std::hash</code>
for custom types. Existing uses of <code>base::hash_map</code>, etc., will be <a
href="https://crbug.com/576864">replaced throughout the codebase</a>. <a
href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/nCdjQqnouO4">Discussion
thread</a>.</td>
On 2016/01/14 21:29:10, danakj wrote:
> "Existing uses of <code>base::hash_map</code>, e
>       tc., will be <a href="https://crbug.com/576864">replaced throughout the
> codebase
>       </a>."
> 
> ^ That part is going to bitrot, I would leave it out.

Done. In fact, yeah, I think the places I copied it from have already bitrotted.

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h
File base/hash.h (right):

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h#newcode48
base/hash.h:48: inline size_t HashInts32(uint32_t value1, uint32_t value2) {
I switched std::size_t to size_t to avoid having to include both cstddef and
stddef.h in the same header as that seemed kinda silly. I'm assuming these are
always going to be the same.

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h#newcode113
base/hash.h:113: struct IntPairHash<std::pair<Type1, Type2>> {
Is there a better way to do this? / Should I be doing this?

I switched it from a two-argument IntPairHash because most of the consumers I
found do something like...

using MyCoolId = std::pair<int, SomeEnumThatIsAlsoAnInt>;
using MyCoolHash = base::hash_set<MyCoolId>;

So it seems better to switch to

using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<MyCoolId>>;

rather than

using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<int,
SomeEnumThatIsAlsoAnInt>>;

But I can switch it back if the old one was better. This is also not actually
used right now, so we can leave it out and expect code to be converted to a call
to HashInts on a call-by-call basis. I don't have strong opinions.

[danakj, 2016-01-19 21:21:55]

https://codereview.chromium.org/1502373009/diff/240001/base/containers/scoped...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1502373009/diff/240001/base/containers/scoped...
base/containers/scoped_ptr_hash_map.h:21: // Deprecated: use std::unordered_map
instead.
Can you link to a bug about replacing this with unordered_map? (Probably need to
file one.)

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h
File base/hash.h (right):

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h#newcode48
base/hash.h:48: inline size_t HashInts32(uint32_t value1, uint32_t value2) {
On 2016/01/16 00:22:23, davidben wrote:
> I switched std::size_t to size_t to avoid having to include both cstddef and
> stddef.h in the same header as that seemed kinda silly. I'm assuming these are
> always going to be the same.

Ya they will, cool.

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h#newcode113
base/hash.h:113: struct IntPairHash<std::pair<Type1, Type2>> {
On 2016/01/16 00:22:23, davidben wrote:
> Is there a better way to do this? / Should I be doing this?
> 
> I switched it from a two-argument IntPairHash because most of the consumers I
> found do something like...
> 
> using MyCoolId = std::pair<int, SomeEnumThatIsAlsoAnInt>;
> using MyCoolHash = base::hash_set<MyCoolId>;
> 
> So it seems better to switch to
> 
> using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<MyCoolId>>;
> 
> rather than
> 
> using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<int,
> SomeEnumThatIsAlsoAnInt>>;
> 
> But I can switch it back if the old one was better. This is also not actually
> used right now, so we can leave it out and expect code to be converted to a
call
> to HashInts on a call-by-call basis. I don't have strong opinions.

I'm uh.. staring at this pretending I know C++ but I just don't see what you've
changed from before.

The old code was:
template<typename Type1, typename Type2>
struct hash<std::pair<Type1, Type2> > {
  std::size_t operator()(std::pair<Type1, Type2> value) const {const {
    return base::HashPair(value.first, value.second);	  36     return
base::HashInts(value.first, value.second);
  }
};


The new code looks the same wrt the templates. That said passing the pair as the
type argument instead of the 2 values does seem ok, and would work for anything
with a .first and .second.. so what am I missing here..?

[danakj, 2016-01-19 21:28:24]

On 2016/01/16 00:22:23, davidben wrote:
> Moved stuff to base/hash.h and renamed as discussed.
> 
> I haven't gotten rid of the old std::pair specialization or IntPairHash. It
does
> seem like there's a lot of them, but I'm not sure since I had to just tackle
> compile failures one-by-one and got to a ScopedPtrHashMap which would have
> required a custom hasher.
> 
> I did, as a result of that exploration, switch IntPairHash to a
single-parameter
> template (see below), but TBH I still find it kind of strange. It's hard to
grep
> for uses of integer pairs since most call sites set up a typedef first. For
> example:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> 
> What do you two think? Shall I keep IntPairHash in there and, in later CLs,
move
> call sites that like that one to it, or get rid of it (keeping the
> BASE_HASH_NAMESPACE::hash std::pair specialization around) and say callers
like
> that should make a little struct and whatnot when migrating to
> std::unordered_map.

I think I'm fine with keeping IntPairHash around and getting rid of the
std::pair specialization.

> https://codereview.chromium.org/1502373009/diff/200001/base/strings/string16.h
> File base/strings/string16.h (right):
> 
>
https://codereview.chromium.org/1502373009/diff/200001/base/strings/string16....
> base/strings/string16.h:189: // platforms, where base::string16 is a typedef
for
> std::wstring.
> On 2016/01/14 21:29:10, danakj wrote:
> > s/typedef/type alias/
> 
> Done.
> 
>
https://codereview.chromium.org/1502373009/diff/200001/styleguide/c++/c++11.html
> File styleguide/c++/c++11.html (right):
> 
>
https://codereview.chromium.org/1502373009/diff/200001/styleguide/c++/c++11.h...
> styleguide/c++/c++11.html:433: <td>Per the <a
> href="https://google.github.io/styleguide/cppguide.html#std_hash">Google style
> guide</a>, specify custom hashers instead of specializing
<code>std::hash</code>
> for custom types. Existing uses of <code>base::hash_map</code>, etc., will be
<a
> href="https://crbug.com/576864">replaced throughout the codebase</a>. <a
>
href="https://groups.google.com/a/chromium.org/forum/#!topic/cxx/nCdjQqnouO4">Discussion
> thread</a>.</td>
> On 2016/01/14 21:29:10, danakj wrote:
> > "Existing uses of <code>base::hash_map</code>, e
> >       tc., will be <a href="https://crbug.com/576864">replaced throughout
the
> > codebase
> >       </a>."
> > 
> > ^ That part is going to bitrot, I would leave it out.
> 
> Done. In fact, yeah, I think the places I copied it from have already
bitrotted.
> 
> https://codereview.chromium.org/1502373009/diff/240001/base/hash.h
> File base/hash.h (right):
> 
> https://codereview.chromium.org/1502373009/diff/240001/base/hash.h#newcode48
> base/hash.h:48: inline size_t HashInts32(uint32_t value1, uint32_t value2) {
> I switched std::size_t to size_t to avoid having to include both cstddef and
> stddef.h in the same header as that seemed kinda silly. I'm assuming these are
> always going to be the same.
> 
> https://codereview.chromium.org/1502373009/diff/240001/base/hash.h#newcode113
> base/hash.h:113: struct IntPairHash<std::pair<Type1, Type2>> {
> Is there a better way to do this? / Should I be doing this?
> 
> I switched it from a two-argument IntPairHash because most of the consumers I
> found do something like...
> 
> using MyCoolId = std::pair<int, SomeEnumThatIsAlsoAnInt>;
> using MyCoolHash = base::hash_set<MyCoolId>;
> 
> So it seems better to switch to
> 
> using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<MyCoolId>>;
> 
> rather than
> 
> using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<int,
> SomeEnumThatIsAlsoAnInt>>;
> 
> But I can switch it back if the old one was better. This is also not actually
> used right now, so we can leave it out and expect code to be converted to a
call
> to HashInts on a call-by-call basis. I don't have strong opinions.

[davidben, 2016-01-19 22:30:28]

https://codereview.chromium.org/1502373009/diff/240001/base/containers/scoped...
File base/containers/scoped_ptr_hash_map.h (right):

https://codereview.chromium.org/1502373009/diff/240001/base/containers/scoped...
base/containers/scoped_ptr_hash_map.h:21: // Deprecated: use std::unordered_map
instead.
On 2016/01/19 21:21:54, danakj wrote:
> Can you link to a bug about replacing this with unordered_map? (Probably need
to
> file one.)

Done.

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h
File base/hash.h (right):

https://codereview.chromium.org/1502373009/diff/240001/base/hash.h#newcode113
base/hash.h:113: struct IntPairHash<std::pair<Type1, Type2>> {
On 2016/01/19 21:21:54, danakj wrote:
> On 2016/01/16 00:22:23, davidben wrote:
> > Is there a better way to do this? / Should I be doing this?
> > 
> > I switched it from a two-argument IntPairHash because most of the consumers
I
> > found do something like...
> > 
> > using MyCoolId = std::pair<int, SomeEnumThatIsAlsoAnInt>;
> > using MyCoolHash = base::hash_set<MyCoolId>;
> > 
> > So it seems better to switch to
> > 
> > using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<MyCoolId>>;
> > 
> > rather than
> > 
> > using MyCoolHash = base::hash_set<MyCoolId, IntPairHash<int,
> > SomeEnumThatIsAlsoAnInt>>;
> > 
> > But I can switch it back if the old one was better. This is also not
actually
> > used right now, so we can leave it out and expect code to be converted to a
> call
> > to HashInts on a call-by-call basis. I don't have strong opinions.
> 
> I'm uh.. staring at this pretending I know C++ but I just don't see what
you've
> changed from before.
> 
> The old code was:
> template<typename Type1, typename Type2>
> struct hash<std::pair<Type1, Type2> > {
>   std::size_t operator()(std::pair<Type1, Type2> value) const {const {
>     return base::HashPair(value.first, value.second);	  36     return
> base::HashInts(value.first, value.second);
>   }
> };
> 
> 
> The new code looks the same wrt the templates. That said passing the pair as
the
> type argument instead of the 2 values does seem ok, and would work for
anything
> with a .first and .second.. so what am I missing here..?

Oh, sorry, that was ambiguous. By "the old one", I meant "a previous patch set's
version of the new code".

https://codereview.chromium.org/1502373009/diff/220001/base/hash.h#pair-109

The difference being whether IntPairHash takes a single template argument that
must be std::pair<T1, T2> or if it takes two template arguments, T1 and T2.

Originally I did the latter, but I think taking the single argument is a bit
better because it's friendlier to typedefs.

[danakj, 2016-01-19 22:41:27]

base and cc LGTM

[danakj, 2016-01-19 22:41:36]

Oh, and styleguide.

[Nico, 2016-01-19 23:37:53]

lgtm 2

Thanks much for doing this :-)

[davidben, 2016-01-20 19:19:56]

Description was changed from

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.
    
BUG=576864
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.
    
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@...
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[davidben, 2016-01-20 19:19:56]

davidben@chromium.org changed reviewers:
+ blundell@chromium.org, dalecurtis@chromium.org, derat@chromium.org,
jbauman@chromium.org

[davidben, 2016-01-20 19:20:56]

Adding a number of OWNERS for TBR. It's all just IWYU violations that this CL
uncovered.

derat for ash/
blundell for components/
jbauman for gpu/command_buffer/
dalecurtis for media/

[davidben, 2016-01-20 19:21:29]

The CQ bit was checked by davidben@chromium.org

[DaleCurtis, 2016-01-20 19:22:33]

media/ lgtm thanks!

[commit-bot: I haz the power, 2016-01-20 19:23:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1502373009/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1502373009/260001

[Daniel Erat, 2016-01-20 19:33:30]

lgtm for ash/

[commit-bot: I haz the power, 2016-01-20 19:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 19:48:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[davidben, 2016-01-20 21:17:09]

On 2016/01/20 19:48:28, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

The std::wstring presubmit check is a bit overzealous it seems. (I mention
wstring in a comment.) Also had to rebase. I'll wait for the try jobs to clear
and then CQ again with NOPRESUBMIT=true.

[davidben, 2016-01-20 23:16:03]

Description was changed from

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.
    
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@...
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.

(NOPRESUBMIT because the wstring presubmit check is overzealous
and complains about the reference to wstring in the comment.)

NOPRESUBMIT=true
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@...
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[davidben, 2016-01-20 23:16:16]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-01-20 23:16:16]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
danakj@chromium.org, thakis@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1502373009/#ps280001
(title: "rebase, more HashPair -> HashInts")

[commit-bot: I haz the power, 2016-01-20 23:17:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1502373009/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1502373009/280001

[commit-bot: I haz the power, 2016-01-21 01:08:46]

Description was changed from

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.

(NOPRESUBMIT because the wstring presubmit check is overzealous
and complains about the reference to wstring in the comment.)

NOPRESUBMIT=true
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@...
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.

(NOPRESUBMIT because the wstring presubmit check is overzealous
and complains about the reference to wstring in the comment.)

NOPRESUBMIT=true
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@...
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-01-21 01:08:47]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-01-21 01:10:18]

Description was changed from

==========
Allow std::unordered_*.
    
base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.
    
Also fix a host of IWYU problems that arose from this CL.

(NOPRESUBMIT because the wstring presubmit check is overzealous
and complains about the reference to wstring in the comment.)

NOPRESUBMIT=true
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@...
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow std::unordered_*.

base::hash_* is, as a transition step, implemented in terms of
std::unordered_*. Later commits will convert existing uses.

Also fix a host of IWYU problems that arose from this CL.

(NOPRESUBMIT because the wstring presubmit check is overzealous
and complains about the reference to wstring in the comment.)

NOPRESUBMIT=true
BUG=576864
TBR=derat@chromium.org,blundell@chromium.org,jbauman@chromium.org,dalecurtis@...
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3f37f7f1459e7b5a452c0e433493e0a6e9649ca7
Cr-Commit-Position: refs/heads/master@{#370553}
==========

[commit-bot: I haz the power, 2016-01-21 01:10:19]

Patchset 15 (id:??) landed as
https://crrev.com/3f37f7f1459e7b5a452c0e433493e0a6e9649ca7
Cr-Commit-Position: refs/heads/master@{#370553}

[davidben, 2016-01-21 01:35:32]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1610023003/ by davidben@chromium.org.

The reason for reverting is: MSan build failure.

https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Linux%20....