WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

This has drawbacks for deleted and empty buckets, but no worse than what
already exists for integral types. Whereas integers reserve 0 and -1 for the
empty and deleted values, respectively, enums reserve -1 and -2 (or the max
and max-1 values, for enums with unsigned underlying types).

Since enumerators start at zero by default and negative enumerators are rare,
this should suffice for most uses.

[jbroman, 2017-02-09 18:15:16]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[jbroman, 2017-02-09 18:15:25]

Description was changed from

==========
WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

This has drawbacks for deleted and empty buckets, but no worse than what
already exists for integral types.

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

to

==========
WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

This has drawbacks for deleted and empty buckets, but no worse than what
already exists for integral types (specifically, 0 and -1 are reserved).

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

[commit-bot: I haz the power, 2017-02-09 18:16:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-09 18:16:09]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 18:16:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-09 18:16:30]

Patchset #1 (id:1) has been deleted

[jbroman, 2017-02-09 18:57:04]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 18:57:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-09 18:58:46]

Description was changed from

==========
WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

This has drawbacks for deleted and empty buckets, but no worse than what
already exists for integral types (specifically, 0 and -1 are reserved).

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

to

==========
WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

This has drawbacks for deleted and empty buckets, but no worse than what
already exists for integral types (specifically, 0 and -1 are reserved).
==========

[jbroman, 2017-02-09 19:41:59]

jbroman@chromium.org changed reviewers:
+ yutak@chromium.org

[jbroman, 2017-02-09 19:45:47]

Luna ran into this when she added a HashMap with an enum key.

Enums are substantially similar to ints from a hashing point of view, so it
seems reasonable to have them be automatically supported in the same way.

The drawback that applies to ints (w.r.t. the default special treatment of 0 and
-1 for empty and deleted, respectively), still applies here -- but we've
apparently decided it's okay for ints.

WDYT?

[commit-bot: I haz the power, 2017-02-09 20:46:04]

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

[commit-bot: I haz the power, 2017-02-09 20:46:05]

Dry run: This issue passed the CQ dry run.

[Yuta Kitamura, 2017-02-10 05:28:57]

On 2017/02/09 19:45:47, jbroman wrote:
> Luna ran into this when she added a HashMap with an enum key.
> 
> Enums are substantially similar to ints from a hashing point of view, so it
> seems reasonable to have them be automatically supported in the same way.
> 
> The drawback that applies to ints (w.r.t. the default special treatment of 0
and
> -1 for empty and deleted, respectively), still applies here -- but we've
> apparently decided it's okay for ints.
> 
> WDYT?

Hmm, I think for enums the value of 0 is very common, so we probably
want to give some warnings. Besides that, I think having this ability
would be generally useful.

[jbroman, 2017-02-10 16:37:47]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 16:38:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-10 16:38:43]

Description was changed from

==========
WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

This has drawbacks for deleted and empty buckets, but no worse than what
already exists for integral types (specifically, 0 and -1 are reserved).
==========

to

==========
WTF: Support enums in DefaultHash and HashTraits by casting to integral types.

This has drawbacks for deleted and empty buckets, but no worse than what
already exists for integral types. Whereas integers reserve 0 and -1 for the
empty and deleted values, respectively, enums reserve -1 and -2 (or the max
and max-1 values, for enums with unsigned underlying types).

Since enumerators start at zero by default and negative enumerators are rare,
this should suffice for most uses.
==========

[jbroman, 2017-02-10 16:59:29]

On 2017/02/10 at 05:28:57, yutak wrote:
> On 2017/02/09 19:45:47, jbroman wrote:
> > Luna ran into this when she added a HashMap with an enum key.
> > 
> > Enums are substantially similar to ints from a hashing point of view, so it
> > seems reasonable to have them be automatically supported in the same way.
> > 
> > The drawback that applies to ints (w.r.t. the default special treatment of 0
and
> > -1 for empty and deleted, respectively), still applies here -- but we've
> > apparently decided it's okay for ints.
> > 
> > WDYT?
> 
> Hmm, I think for enums the value of 0 is very common, so we probably
> want to give some warnings. Besides that, I think having this ability
> would be generally useful.

You're absolutely right; I hadn't realized that both the empty and deleted
values
were forbidden from being hash keys. I've changed to -1 and -2 because, as you
point out, enumerators start at zero by default. PTAL.

[commit-bot: I haz the power, 2017-02-10 17:42:01]

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

[commit-bot: I haz the power, 2017-02-10 17:42:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jbroman, 2017-02-10 18:23:45]

On 2017/02/10 at 16:59:29, jbroman wrote:
> On 2017/02/10 at 05:28:57, yutak wrote:
> > On 2017/02/09 19:45:47, jbroman wrote:
> > > Luna ran into this when she added a HashMap with an enum key.
> > > 
> > > Enums are substantially similar to ints from a hashing point of view, so
it
> > > seems reasonable to have them be automatically supported in the same way.
> > > 
> > > The drawback that applies to ints (w.r.t. the default special treatment of
0 and
> > > -1 for empty and deleted, respectively), still applies here -- but we've
> > > apparently decided it's okay for ints.
> > > 
> > > WDYT?
> > 
> > Hmm, I think for enums the value of 0 is very common, so we probably
> > want to give some warnings. Besides that, I think having this ability
> > would be generally useful.
> 
> You're absolutely right; I hadn't realized that both the empty and deleted
values
> were forbidden from being hash keys. I've changed to -1 and -2 because, as you
> point out, enumerators start at zero by default. PTAL.

Hold on: there are cases where we're storing bitfields as enums, where this
seems to be triggering. :(

[jbroman, 2017-02-10 19:02:06]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 19:03:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 20:42:38]

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

[commit-bot: I haz the power, 2017-02-10 20:42:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2017-02-13 15:12:33]

On 2017/02/10 at 18:23:45, jbroman wrote:
> On 2017/02/10 at 16:59:29, jbroman wrote:
> > On 2017/02/10 at 05:28:57, yutak wrote:
> > > On 2017/02/09 19:45:47, jbroman wrote:
> > > > Luna ran into this when she added a HashMap with an enum key.
> > > > 
> > > > Enums are substantially similar to ints from a hashing point of view, so
it
> > > > seems reasonable to have them be automatically supported in the same
way.
> > > > 
> > > > The drawback that applies to ints (w.r.t. the default special treatment
of 0 and
> > > > -1 for empty and deleted, respectively), still applies here -- but we've
> > > > apparently decided it's okay for ints.
> > > > 
> > > > WDYT?
> > > 
> > > Hmm, I think for enums the value of 0 is very common, so we probably
> > > want to give some warnings. Besides that, I think having this ability
> > > would be generally useful.
> > 
> > You're absolutely right; I hadn't realized that both the empty and deleted
values
> > were forbidden from being hash keys. I've changed to -1 and -2 because, as
you
> > point out, enumerators start at zero by default. PTAL.
> 
> Hold on: there are cases where we're storing bitfields as enums, where this
seems to be triggering. :(

Hmm. Changing the empty value to something else is risky, but not doing it
causes problems for a common set of enums. Maybe not providing a default here is
best. :/

[Yuta Kitamura, 2017-02-14 05:10:24]

On 2017/02/13 15:12:33, jbroman wrote:
> Hmm. Changing the empty value to something else is risky, but not doing it
> causes problems for a common set of enums. Maybe not providing a default here
is
> best. :/

Yeah, that might explain why we don't have that today...