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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 30 22:35:53 PST 2022

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 235872: [WTF] Add GenericHashKey

Attachment 450380: Patch


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

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.

More information about the webkit-reviews mailing list