Bug 71063 - HashMap<>::add should return a more descriptive object
Summary: HashMap<>::add should return a more descriptive object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-27 14:24 PDT by Ryosuke Niwa
Modified: 2012-03-29 11:49 PDT (History)
19 users (show)

See Also:


Attachments
Patch (150.69 KB, patch)
2012-03-27 18:46 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (168.87 KB, patch)
2012-03-28 11:17 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (169.73 KB, patch)
2012-03-28 11:31 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (173.09 KB, patch)
2012-03-28 13:32 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (174.17 KB, patch)
2012-03-28 16:15 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (176.09 KB, patch)
2012-03-29 05:53 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (177.00 KB, patch)
2012-03-29 06:14 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (184.41 KB, patch)
2012-03-29 08:22 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (185.58 KB, patch)
2012-03-29 09:25 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (186.14 KB, patch)
2012-03-29 10:11 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-10-27 14:24:56 PDT
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.
Comment 1 Caio Marcelo de Oliveira Filho 2012-01-06 16:22:30 PST
(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;
Comment 2 Ryosuke Niwa 2012-01-06 16:45:38 PST
(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;
Comment 3 Caio Marcelo de Oliveira Filho 2012-03-27 18:46:21 PDT
Created attachment 134195 [details]
Patch
Comment 4 Alexis Menard (darktears) 2012-03-27 19:01:16 PDT
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?
Comment 5 Philippe Normand 2012-03-27 19:07:33 PDT
Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12140886
Comment 6 Build Bot 2012-03-27 19:07:43 PDT
Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12140883
Comment 7 Build Bot 2012-03-27 19:08:37 PDT
Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12148750
Comment 8 Gyuyoung Kim 2012-03-27 19:22:37 PDT
Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12143899
Comment 9 WebKit Review Bot 2012-03-27 19:39:48 PDT
Comment on attachment 134195 [details]
Patch

Attachment 134195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12148762
Comment 10 Caio Marcelo de Oliveira Filho 2012-03-28 06:40:49 PDT
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.
Comment 11 Caio Marcelo de Oliveira Filho 2012-03-28 11:17:44 PDT
Created attachment 134345 [details]
Patch
Comment 12 Caio Marcelo de Oliveira Filho 2012-03-28 11:31:27 PDT
Created attachment 134350 [details]
Patch
Comment 13 Build Bot 2012-03-28 11:56:54 PDT
Comment on attachment 134350 [details]
Patch

Attachment 134350 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12151342
Comment 14 Ryosuke Niwa 2012-03-28 12:02:45 PDT
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?
Comment 15 Build Bot 2012-03-28 12:55:29 PDT
Comment on attachment 134350 [details]
Patch

Attachment 134350 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12157196
Comment 16 Build Bot 2012-03-28 13:08:05 PDT
Comment on attachment 134350 [details]
Patch

Attachment 134350 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12159247
Comment 17 Caio Marcelo de Oliveira Filho 2012-03-28 13:32:19 PDT
Created attachment 134383 [details]
Patch
Comment 18 Caio Marcelo de Oliveira Filho 2012-03-28 13:40:57 PDT
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!
Comment 19 Build Bot 2012-03-28 14:08:29 PDT
Comment on attachment 134383 [details]
Patch

Attachment 134383 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12156307
Comment 20 Build Bot 2012-03-28 16:08:42 PDT
Comment on attachment 134383 [details]
Patch

Attachment 134383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12153495
Comment 21 Caio Marcelo de Oliveira Filho 2012-03-28 16:15:30 PDT
Created attachment 134435 [details]
Patch
Comment 22 Ryosuke Niwa 2012-03-28 16:21:56 PDT
Comment on attachment 134435 [details]
Patch

Thanks for the patch! New code looks great to me.
Comment 23 Build Bot 2012-03-28 16:53:42 PDT
Comment on attachment 134435 [details]
Patch

Attachment 134435 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12157332
Comment 24 Caio Marcelo de Oliveira Filho 2012-03-29 05:53:11 PDT
Created attachment 134556 [details]
Patch
Comment 25 Caio Marcelo de Oliveira Filho 2012-03-29 06:14:49 PDT
Created attachment 134558 [details]
Patch
Comment 26 Build Bot 2012-03-29 06:55:39 PDT
Comment on attachment 134558 [details]
Patch

Attachment 134558 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12193301
Comment 27 Caio Marcelo de Oliveira Filho 2012-03-29 08:22:05 PDT
Created attachment 134592 [details]
Patch
Comment 28 Caio Marcelo de Oliveira Filho 2012-03-29 09:25:09 PDT
Created attachment 134599 [details]
Patch
Comment 29 Build Bot 2012-03-29 10:07:18 PDT
Comment on attachment 134599 [details]
Patch

Attachment 134599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12212026
Comment 30 Caio Marcelo de Oliveira Filho 2012-03-29 10:11:40 PDT
Created attachment 134610 [details]
Patch
Comment 31 Caio Marcelo de Oliveira Filho 2012-03-29 11:49:38 PDT
Committed r112555: <http://trac.webkit.org/changeset/112555>