Bug 235872 – [WTF] Add GenericHashKey

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Add GenericHashKey

[Yusuke Suzuki]

Created attachment 450363 [details]
Patch

[Yusuke Suzuki]

Created attachment 450364 [details]
Patch

[Darin Adler]

I like this idea, helping fit any type into our hash table that requires empty and deleted values, not as sure about the name. If I think of a better name I will suggest it.

[Yusuke Suzuki]

Created attachment 450380 [details]
Patch

[Yusuke Suzuki]

(In reply to Darin Adler from comment #3)
> I like this idea, helping fit any type into our hash table that requires
> empty and deleted values, not as sure about the name. If I think of a better
> name I will suggest it.

Nice!

[Darin Adler]

Comment on attachment 450380 [details]
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.

[Yusuke Suzuki]

Comment on attachment 450380 [details]
Patch

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

>> 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.

For now, in this patch, I'll just use the existing interface.

>> Source/JavaScriptCore/runtime/TypeLocationCache.cpp:59
>> +        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.

Sounds good! Changed.

>> Source/WTF/wtf/GenericHashKey.h:77
>> +            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.

Sounds good, changed!

[Yusuke Suzuki]

Committed r288807 (246583@trunk): <https://commits.webkit.org/246583@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/88257247>