[webkit-reviews] review granted: [Bug 235872] [WTF] Add GenericHashKey : [Attachment 450380] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 235872: [WTF] Add GenericHashKey
https://bugs.webkit.org/show_bug.cgi?id=235872

Attachment 450380: Patch

https://bugs.webkit.org/attachment.cgi?id=450380&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 450380
  --> https://bugs.webkit.org/attachment.cgi?id=450380
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450380&action=review

Can’t help thinking how much better some of this code would look with more use
of auto, auto*, and auto&.

> Source/JavaScriptCore/dfg/DFGIntegerCheckCombiningPhase.cpp:195
> +	       Range& range = m_map.add(data.m_key, Range { }).iterator->value;

When I see call sites like this, I wonder if we should overload add to have the
empty value as the default. It’s annoying to have to write Range { } here.

> Source/JavaScriptCore/runtime/TypeLocationCache.cpp:59
> +    } else
> +	   location = iterator->value;

I often like to reverse these things so that the shorter half of the if
statement goes first.

But really, this looks like it should be rewritten to use ensure to me.

> Source/WTF/wtf/GenericHashKey.h:77
> +	   if (m_value.index() != 0)
> +	       return true;

Why not write this using holds_alternative instead of index? I think it would
compile to the exact same code, but it would not depend on the ordering of the
alternative types in the variant.