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.
(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;
(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;
Created attachment 134195 [details] Patch
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 on attachment 134195 [details] Patch Attachment 134195 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12140886
Comment on attachment 134195 [details] Patch Attachment 134195 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12140883
Comment on attachment 134195 [details] Patch Attachment 134195 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12148750
Comment on attachment 134195 [details] Patch Attachment 134195 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12143899
Comment on attachment 134195 [details] Patch Attachment 134195 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12148762
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.
Created attachment 134345 [details] Patch
Created attachment 134350 [details] Patch
Comment on attachment 134350 [details] Patch Attachment 134350 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12151342
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 on attachment 134350 [details] Patch Attachment 134350 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12157196
Comment on attachment 134350 [details] Patch Attachment 134350 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12159247
Created attachment 134383 [details] Patch
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 on attachment 134383 [details] Patch Attachment 134383 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12156307
Comment on attachment 134383 [details] Patch Attachment 134383 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12153495
Created attachment 134435 [details] Patch
Comment on attachment 134435 [details] Patch Thanks for the patch! New code looks great to me.
Comment on attachment 134435 [details] Patch Attachment 134435 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12157332
Created attachment 134556 [details] Patch
Created attachment 134558 [details] Patch
Comment on attachment 134558 [details] Patch Attachment 134558 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12193301
Created attachment 134592 [details] Patch
Created attachment 134599 [details] Patch
Comment on attachment 134599 [details] Patch Attachment 134599 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12212026
Created attachment 134610 [details] Patch
Committed r112555: <http://trac.webkit.org/changeset/112555>