Align base::hash_map with C++11, part 2.

Align base::hash_map with C++11, part 2.

MSVC's stdext::hash_map, unlike C++11's std::unordered_map and GCC's
__gnu_cxx::hash_map, requires a total order on the key. It also has a very
different syntax for supplying a default hash function.

Switch MSVC to std::unordered_map, but exposed as base::hash_map. This aligns
both the container and the default hash function. The default hash function now
differs from our GCC one in that const char * is no longer hashed as a C string.

To align GCC with C++11 semantics and the MSVC ones this CL introduces, also
provide a new hash function and swap the default hash with it. This new
function is identical to __gnu_cxx::hash, but it does not specialize const
char* and does specialize T* (which was already aligned by
https://codereview.chromium.org/630503002/).

Note: This CL changes a file in mojo/ and will need to be mirrored in the
mojo repository before the next sync.

BUG=420242
Committed: https://crrev.com/44d2e5c45975ab36f29035552521e41ae512c8bf
Cr-Commit-Position: refs/heads/master@{#301185}

[davidben, 2014-10-06 19:17:06]

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

[davidben, 2014-10-06 19:17:08]

There wasn't much chatter on the thread I sent out, so I guess I'll publish it
for review and see what people think? Not sure what the process is here as it's
not strictly a C++11 feature.

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/cjiUwbUhQXM

This depends on:
https://codereview.chromium.org/630503002/

It also depends on being allowed to use C++11 type aliases, so we'll either need
another constructor, let that one through anyway, or wait until type aliases get
approved.

https://codereview.chromium.org/623383002/diff/1/base/containers/hash_tables.h
File base/containers/hash_tables.h (right):

https://codereview.chromium.org/623383002/diff/1/base/containers/hash_tables....
base/containers/hash_tables.h:145: using hash_map = std::unordered_map<Key, T,
Hash, Pred, Alloc>;
Note: This uses a feature that hasn't been approved yet: type aliases. Not sure
how to accomplish the renaming here and the default hash swap below without it.

[davidben, 2014-10-06 23:37:24]

Transplanting some comments from the original CL.

https://codereview.chromium.org/623383002/diff/1/base/containers/small_map_un...
File base/containers/small_map_unittest.cc (left):

https://codereview.chromium.org/623383002/diff/1/base/containers/small_map_un...
base/containers/small_map_unittest.cc:417: hash_map_add_item() : hash_map<int,
int>() {}
This is to work around an incredibly annoying problem with type aliases. If we
were to call it base::unordered_map, I think we would also avoid this problem.

The trouble is that we're subclassing base::hash_map<int, int>. But within
base::hash_map, 'hash_map' is bound to std::hash_map. And so inside the
subclass, we inherit the parent class's binding of 'hash_map' over the one in
base. I believe this is similar why this works:

  namespace a {
  struct Foo {};
  }
  struct Bar : public a::Foo {
    Bar() : Foo() {}
  };

And this:

  namespace a {
  struct Foo {};
  typedef Foo Foo2;
  }
  struct Bar : public a::Foo2 {
    Bar() : Foo() {}
  };

But not this:

  namespace a {
  struct Foo {};
  typedef Foo Foo2;
  }
  struct Bar : public a::Foo2 {
    Bar() : Foo2() {}
  };


I'm going to leave it as-is for now since subclassing hash_map is a bizarre
thing to do, but we may want do the rename now to avoid this.

https://codereview.chromium.org/623383002/diff/1/content/renderer/pepper/v8_v...
File content/renderer/pepper/v8_var_converter.cc (left):

https://codereview.chromium.org/623383002/diff/1/content/renderer/pepper/v8_v...
content/renderer/pepper/v8_var_converter.cc:50: bool operator<(const
HashedHandle& h) const { return hash() < h.hash(); }
I left the other operator<'s in place because it wasn't immediately clear
whether a std::map of them would have made sense in each case...

...except this one. This one looks pretty clearly nonsense. Unless
GetIdentityHash() is secretly GetIdentityUniqueValue(), we weren't handling hash
collisions on MSVC's hash_map. It doesn't use operator==.

[danakj, 2014-10-09 00:05:10]

On Mon, Oct 6, 2014 at 3:17 PM, <davidben@chromium.org> wrote:

> Reviewers: danakj,
>
> Message:
> There wasn't much chatter on the thread I sent out, so I guess I'll
> publish it
> for review and see what people think? Not sure what the process is here as
> it's
> not strictly a C++11 feature.
>
> https://groups.google.com/a/chromium.org/forum/#!topic/
> chromium-dev/cjiUwbUhQXM
>
> This depends on:
> https://codereview.chromium.org/630503002/
>
> It also depends on being allowed to use C++11 type aliases, so we'll
> either need
> another constructor, let that one through anyway, or wait until type
> aliases get
> approved.
>

Have they been proposed? If not can you propose them? If they have can you
try push that forward?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[davidben, 2014-10-09 18:14:25]

On 2014/10/09 00:05:10, danakj wrote:
> On Mon, Oct 6, 2014 at 3:17 PM, <mailto:davidben@chromium.org> wrote:
> 
> > Reviewers: danakj,
> >
> > Message:
> > There wasn't much chatter on the thread I sent out, so I guess I'll
> > publish it
> > for review and see what people think? Not sure what the process is here as
> > it's
> > not strictly a C++11 feature.
> >
> > https://groups.google.com/a/chromium.org/forum/#!topic/
> > chromium-dev/cjiUwbUhQXM
> >
> > This depends on:
> > https://codereview.chromium.org/630503002/
> >
> > It also depends on being allowed to use C++11 type aliases, so we'll
> > either need
> > another constructor, let that one through anyway, or wait until type
> > aliases get
> > approved.
> >
> 
> Have they been proposed? If not can you propose them? If they have can you
> try push that forward?

There's a thread, though there hasn't been much noise on it. I'll +1 it. Another
option that seems to work is to subclass instead of alias things:

template<class Key, class T,
         class Hash = BASE_HASH_NAMESPACE::hash<Key>,
         class Pred = std::equal_to<Key>,
         class Alloc = std::allocator<std::pair<const Key, T>>>
class hash_map : public BASE_HASH_IMPL_NAMESPACE::hash_map<Key, T, Hash, Pred,
Alloc> {
};

But if we can get type aliases allowed soon, that seems cleaner. I'll respond on
the thread.

[davidben, 2014-10-09 18:18:20]

On 2014/10/09 18:14:25, David Benjamin wrote:
> There's a thread, though there hasn't been much noise on it. I'll +1 it.
Another
> option that seems to work is to subclass instead of alias things:

(Pfft. Alternatively, Gmail collapsing messages is confusing. Looks like it's in
the queue, but I'll ping the thread.)

[davidben, 2014-10-10 00:32:08]

On 2014/10/09 18:18:20, David Benjamin wrote:
> On 2014/10/09 18:14:25, David Benjamin wrote:
> > There's a thread, though there hasn't been much noise on it. I'll +1 it.
> Another
> > option that seems to work is to subclass instead of alias things:
> 
> (Pfft. Alternatively, Gmail collapsing messages is confusing. Looks like it's
in
> the queue, but I'll ping the thread.)

Type aliases are now allowed in https://chromium-cpp.appspot.com/

[davidben, 2014-10-15 19:05:12]

friendly ping

[davidben, 2014-10-20 22:20:50]

davidben@chromium.org changed reviewers:
+ mark@chromium.org

[davidben, 2014-10-20 22:20:51]

+mark for review (danakj tells me she's busy).

[Mark Mentovai, 2014-10-21 03:34:17]

James is the only person who weighed in with concerns on your thread, so let’s
give him a chance to comment.

[davidben, 2014-10-21 16:06:22]

davidben@chromium.org changed reviewers:
+ jamesr@chromium.org

[davidben, 2014-10-21 16:06:22]

Good point. +jamesr for review then.

[jamesr, 2014-10-22 00:18:33]

My main concern is that this changes a lot at once, which increases the risk of
subtle bugs.  I don't think we can break this up any further though.

lgtm (you'll still need OWNERS for base/ etc)

[Mark Mentovai, 2014-10-22 16:28:32]

LGTM

[davidben, 2014-10-22 18:23:31]

davidben@chromium.org changed reviewers:
+ brettw@chromium.org, sky@chromium.org

[davidben, 2014-10-22 18:23:31]

Aaand for the OWNERS monster.

sky: chrome/
brettw: tools/gn/

[danakj, 2014-10-22 18:25:52]

cc LGTM

[sky, 2014-10-22 20:21:14]

LGTM

https://codereview.chromium.org/623383002/diff/110001/chrome/browser/browsing...
File chrome/browser/browsing_data/canonical_cookie_hash.h (left):

https://codereview.chromium.org/623383002/diff/110001/chrome/browser/browsing...
chrome/browser/browsing_data/canonical_cookie_hash.h:12: #if
defined(COMPILER_MSVC)
Remove any includes that are now stale.

[davidben, 2014-10-22 21:06:56]

https://codereview.chromium.org/623383002/diff/110001/chrome/browser/browsing...
File chrome/browser/browsing_data/canonical_cookie_hash.h (left):

https://codereview.chromium.org/623383002/diff/110001/chrome/browser/browsing...
chrome/browser/browsing_data/canonical_cookie_hash.h:12: #if
defined(COMPILER_MSVC)
On 2014/10/22 20:21:14, sky wrote:
> Remove any includes that are now stale.

Done.

Hrm, that block probably didn't even work anyway since it wasn't including
compiler_specific.h. Did a pass and removed some other stale includes.

[davidben, 2014-10-24 01:16:30]

brettw: friendly ping

[brettw, 2014-10-24 16:51:46]

tools/gn LGTM

[davidben, 2014-10-24 18:10:00]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-10-24 18:10:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/623383002/130001

[commit-bot: I haz the power, 2014-10-24 20:20:05]

Committed patchset #6 (id:130001)

[commit-bot: I haz the power, 2014-10-24 20:20:55]

Patchset 6 (id:??) landed as
https://crrev.com/44d2e5c45975ab36f29035552521e41ae512c8bf
Cr-Commit-Position: refs/heads/master@{#301185}