Bug 71063 – HashMap<>::add should return a more descriptive object

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ryosuke Niwa]

HashMap<>::add returns a pair of HashMap iterator and a boolean but the semantics of this return value is totally unclear in its call sites.

We end up code like:

pair<EventListenerHashMap::iterator, bool> result = m_hashMap->add(eventType, 0);
if (result.second)
    result.first->second = new EventListenerVector;

We should create some struct for this purpose so that semantics is clearer.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #0)
> We end up code like:
> 
> pair<EventListenerHashMap::iterator, bool> result = m_hashMap->add(eventType, 0);
> if (result.second)
>     result.first->second = new EventListenerVector;

What about have the following?

EventListenerHashMap::AddResult result = m_hashMap->add(eventType, 0);
if (result.isNewKey)
    result.iterator->second = new EventListenerVector;

[Ryosuke Niwa]

(In reply to comment #1)
> (In reply to comment #0)
> > We end up code like:
> > 
> > pair<EventListenerHashMap::iterator, bool> result = m_hashMap->add(eventType, 0);
> > if (result.second)
> >     result.first->second = new EventListenerVector;
> 
> What about have the following?
> 
> EventListenerHashMap::AddResult result = m_hashMap->add(eventType, 0);
> if (result.isNewKey)
>     result.iterator->second = new EventListenerVector;

Yes, that'll be an improvement. We should probably improve the interface of iterator as well so that it'll be something like:

if (result.isNewKey)
    result.iterator->value = new EventListenerVector;

[Caio Marcelo de Oliveira Filho]

Created attachment 134195 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 134195 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        the iterator type, there's a need for its own AddResult type instead of a simple

Is there any other benefit that a better readability?

> Source/WTF/wtf/HashTable.h:335
> +            iterator iter;

not sure if iter is a good name. We usually use it no?

[Philippe Normand]

Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12140886

[Build Bot]

Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12140883

[Build Bot]

Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12148750

[Gyuyoung Kim]

Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12143899

[WebKit Review Bot]

Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12148762

[Caio Marcelo de Oliveira Filho]

Comment on attachment 134195 [details]
Patch

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

I'll upload a new version fixing the EWS results.

>> Source/JavaScriptCore/ChangeLog:9
>> +        the iterator type, there's a need for its own AddResult type instead of a simple
> 
> Is there any other benefit that a better readability?

No. As the WTF/ChangeLog pointed out, this change's goal is to improve readability. In practice, this is just a specialized pair.

>> Source/WTF/wtf/HashTable.h:335
>> +            iterator iter;
> 
> not sure if iter is a good name. We usually use it no?

I prefer iter, I would even prefer iterator. I will give it a try here. I think 'it' works better as a local variable with a small scope.

[Caio Marcelo de Oliveira Filho]

Created attachment 134345 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 134350 [details]
Patch

[Build Bot]

Comment on attachment 134350 [details]
Patch

Attachment 134350 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12151342

[Ryosuke Niwa]

Comment on attachment 134350 [details]
Patch

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

> Source/JavaScriptCore/API/JSCallbackObject.h:92
> -            m_propertyMap.add(propertyName.impl(), empty).first->second.set(globalData, owner, value);
> +            m_propertyMap.add(propertyName.impl(), empty).iter->second.set(globalData, owner, value);

Can we add key() and value() to AddResult so that we can avoid using first/second?
In the long term, we should also add specialized pair for iterators.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:972
> -    if (result.second) // new entry
> +    IdentifierMap::AddResult result = m_identifierMap.add(rep, m_codeBlock->numberOfIdentifiers());
> +    if (result.isNewKey)

Maybe we should call it isNewEntry instead of isNewKey?

> Source/JavaScriptCore/runtime/WeakGCMap.h:-59
> -
>  public:
> -

Do we really want to remove these blank lines?

> Source/WTF/wtf/ListHashSet.h:88
> +        struct AddResult {
> +            AddResult(iterator iter, bool isNewKey) : iter(iter), isNewKey(isNewKey) { }
> +            iterator iter;
> +            bool isNewKey;
> +        };

It's not great that we have to define multiple versions of AddResult. Can we templatize this class and share across hash classes?

[Build Bot]

Comment on attachment 134350 [details]
Patch

Attachment 134350 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12157196

[Build Bot]

Comment on attachment 134350 [details]
Patch

Attachment 134350 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12159247

[Caio Marcelo de Oliveira Filho]

Created attachment 134383 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 134350 [details]
Patch

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

>> Source/JavaScriptCore/API/JSCallbackObject.h:92
>> +            m_propertyMap.add(propertyName.impl(), empty).iter->second.set(globalData, owner, value);
> 
> Can we add key() and value() to AddResult so that we can avoid using first/second?
> In the long term, we should also add specialized pair for iterators.

We can, but I'd rather not do it right now. Let's try first add specialized pair for iterators. If we manage to get that, we might not want to have key() since will cause potential confusion with iterator::key (which is not a function). If it happens that we want the shortcuts anyway, we can add them after.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:972
>> +    if (result.isNewKey)
> 
> Maybe we should call it isNewEntry instead of isNewKey?

Done.

>> Source/JavaScriptCore/runtime/WeakGCMap.h:-59
>> -
> 
> Do we really want to remove these blank lines?

Fixed.

>> Source/WTF/wtf/ListHashSet.h:88
>> +        };
> 
> It's not great that we have to define multiple versions of AddResult. Can we templatize this class and share across hash classes?

Good idea. The new patch implements this. Since AddResult was moved outside HashTable, we can use 'iterator' instead of 'iter' too!

[Build Bot]

Comment on attachment 134383 [details]
Patch

Attachment 134383 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12156307

[Build Bot]

Comment on attachment 134383 [details]
Patch

Attachment 134383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12153495

[Caio Marcelo de Oliveira Filho]

Created attachment 134435 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 134435 [details]
Patch

Thanks for the patch! New code looks great to me.

[Build Bot]

Comment on attachment 134435 [details]
Patch

Attachment 134435 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12157332

[Caio Marcelo de Oliveira Filho]

Created attachment 134556 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 134558 [details]
Patch

[Build Bot]

Comment on attachment 134558 [details]
Patch

Attachment 134558 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12193301

[Caio Marcelo de Oliveira Filho]

Created attachment 134592 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 134599 [details]
Patch

[Build Bot]

Comment on attachment 134599 [details]
Patch

Attachment 134599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12212026

[Caio Marcelo de Oliveira Filho]

Created attachment 134610 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Committed r112555: <http://trac.webkit.org/changeset/112555>