Bug 121310 – HashMap should work with move-only keys

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

[Anders Carlsson]

HashMap should work with move-only keys

[Anders Carlsson]

Created attachment 211568 [details]
Patch

[WebKit Commit Bot]

Attachment 211568 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/HashMap.h', u'Source/WTF/wtf/HashTable.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp']" exit_code: 1
Source/WTF/wtf/HashMap.h:102:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:107:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:138:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:341:  Missing spaces around >>  [whitespace/operators] [3]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1808071

[Early Warning System Bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1911042

[Build Bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1801228

[kov's GTK+ EWS bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1862233

[Build Bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1878084

[Build Bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1808089

[EFL EWS Bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1875220

[EFL EWS Bot]

Comment on attachment 211568 [details]
Patch

Attachment 211568 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1896179

[Anders Carlsson]

Created attachment 211844 [details]
Patch

[WebKit Commit Bot]

Attachment 211844 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/HashMap.h', u'Source/WTF/wtf/HashTable.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp']" exit_code: 1
Source/WTF/wtf/HashTable.h:405:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:103:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:108:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:109:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:132:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:140:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:143:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:345:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:358:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:378:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:379:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:392:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 12 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 211844 [details]
Patch

Attachment 211844 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1824129

[Anders Carlsson]

Created attachment 211848 [details]
Patch

[WebKit Commit Bot]

Attachment 211848 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/HashMap.h', u'Source/WTF/wtf/HashTable.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderRegion.cpp', u'Source/WebCore/rendering/RenderRegion.h', u'Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp', u'Source/WebCore/svg/SVGDocumentExtensions.cpp', u'Source/WebCore/svg/SVGDocumentExtensions.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp']" exit_code: 1
Source/WTF/wtf/HashTable.h:405:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:103:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:108:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:109:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:132:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:140:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:143:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:345:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:358:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:378:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:379:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:392:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 12 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Created attachment 211849 [details]
Patch

[WebKit Commit Bot]

Attachment 211849 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/HashMap.h', u'Source/WTF/wtf/HashTable.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp', u'Source/WebCore/rendering/RenderRegion.cpp', u'Source/WebCore/rendering/RenderRegion.h', u'Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp', u'Source/WebCore/svg/SVGDocumentExtensions.cpp', u'Source/WebCore/svg/SVGDocumentExtensions.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp']" exit_code: 1
Source/WTF/wtf/HashTable.h:405:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:103:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:108:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:109:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:132:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:140:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:143:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:345:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:358:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:378:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:379:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:392:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 12 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 211849 [details]
Patch

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

> Source/WTF/wtf/HashMap.h:428
> -    auto HashMap<T, U, V, W, MappedTraits>::take(const KeyType& key) -> MappedPassOutType
> +    auto HashMap<T, U, V, W, MappedTraits>::take(const KeyType& key) -> MappedType

So this doesn’t break callers that expect the return type to be PassRefPtr?

[Build Bot]

Comment on attachment 211849 [details]
Patch

Attachment 211849 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1870201

[EFL EWS Bot]

Comment on attachment 211849 [details]
Patch

Attachment 211849 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1919100

[EFL EWS Bot]

Comment on attachment 211849 [details]
Patch

Attachment 211849 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1824186

[kov's GTK+ EWS bot]

Comment on attachment 211849 [details]
Patch

Attachment 211849 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1872259

[Anders Carlsson]

(In reply to comment #18)
> (From update of attachment 211849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211849&action=review
> 
> > Source/WTF/wtf/HashMap.h:428
> > -    auto HashMap<T, U, V, W, MappedTraits>::take(const KeyType& key) -> MappedPassOutType
> > +    auto HashMap<T, U, V, W, MappedTraits>::take(const KeyType& key) -> MappedType
> 
> So this doesn’t break callers that expect the return type to be PassRefPtr?

It does, but I don’t think there were any callers that expected that.

[Anders Carlsson]

Created attachment 211911 [details]
Patch

[WebKit Commit Bot]

Attachment 211911 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/HashMap.h', u'Source/WTF/wtf/HashTable.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/InputType.cpp', u'Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFace.cpp', u'Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp', u'Source/WebCore/rendering/RenderRegion.cpp', u'Source/WebCore/rendering/RenderRegion.h', u'Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp', u'Source/WebCore/svg/SVGDocumentExtensions.cpp', u'Source/WebCore/svg/SVGDocumentExtensions.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp']" exit_code: 1
Source/WTF/wtf/HashTable.h:405:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebCore/html/InputType.cpp:95:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WTF/wtf/HashMap.h:103:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:108:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:109:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:132:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:140:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:143:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:345:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:358:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:378:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/HashMap.h:379:  Missing spaces around >>  [whitespace/operators] [3]
Source/WTF/wtf/HashMap.h:392:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 13 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 211911 [details]
Patch

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

>> Source/WTF/wtf/HashMap.h:140
>> +        template<typename K, typename V>
>> +        AddResult inlineSet(K&&, V&&);
> 
> Missing spaces around &&  [whitespace/operators] [3]

Might read better on one line instead of two.

>> Source/WTF/wtf/HashMap.h:143
>> +        template<typename K, typename V>
>> +        AddResult inlineAdd(K&&, V&&);
> 
> Missing spaces around &&  [whitespace/operators] [3]

Might read better on one line instead of two.

> Source/WebCore/html/InputType.cpp:90
> -static PassOwnPtr<InputTypeFactoryMap> createInputTypeFactoryMap()
> +static OwnPtr<InputTypeFactoryMap> createInputTypeFactoryMap()

The pattern here should change so we aren’t calling HashMap::add repeatedly, since those calls will be inlined. Need to do this in a loop instead. I guess I can make that change after you land. I would suggest we make a static const table that has two function pointers, one to the function that returns the type name and the other being the create function.

[Anders Carlsson]

Committed r155963: <http://trac.webkit.org/changeset/155963>

[Brent Fulgham]

This patch broke the Windows build:

        WebKitGraphics.cpp
     1>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\private\wtf/HashMap.h(238): error C2679: binary '=' : no operator found which takes a right-hand operand of type 'int' (or there is no acceptable conversion)
                 C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\WebCore/COMPtr.h(84): could be 'COMPtr<T> &COMPtr<T>::operator =(const COMPtr<T> &)'
                 with
                 [
                     T=IClassFactory
                 ]
                 C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\WebCore/COMPtr.h(85): or       'COMPtr<T> &COMPtr<T>::operator =(T *)'
                 with
                 [
                     T=IClassFactory
                 ]
                 while trying to match the argument list '(COMPtr<T>, int)'
                 with
                 [
                     T=IClassFactory
                 ]
                 C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\private\wtf/HashTable.h(889) : see reference to function template instantiation 'void WTF::HashMapTranslator<ValueTraits,HashFunctions>::translate<WTF::KeyValuePair<KeyTypeArg,ValueTypeArg>,const _GUID&,T&>(WTF::KeyValuePair<KeyTypeArg,ValueTypeArg> &,U,V)' being compiled
                 with
                 [
                     ValueTraits=WTF::HashMapValueTraits<CLSIDHashTraits,WTF::HashTraits<COMPtr<IClassFactory>>>,
                     HashFunctions=CLSIDHash,
                     KeyTypeArg=_GUID,
                     ValueTypeArg=COMPtr<IClassFactory>,
                     T=int,
                     U=const _GUID &,
                     V=int &
                 ]
                 C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\private\wtf/HashMap.h(358) : see reference to function template instantiation 'WTF::HashTableAddResult<IteratorType> WTF::HashTable<Key,Value,Extractor,HashFunctions,Traits,KeyTraits>::add<WTF::HashMapTranslator<ValueTraits,HashFunctions>,const _GUID&,T&>(const _GUID,Extra)' being compiled
                 with
                 [
                     IteratorType=WTF::HashTableIterator<_GUID,WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>>,CLSIDHash,WTF::HashMapValueTraits<CLSIDHashTraits,WTF::HashTraits<COMPtr<IClassFactory>>>,CLSIDHashTraits>,
                     Key=_GUID,
                     Value=WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>,
                     Extractor=WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>>,
                     HashFunctions=CLSIDHash,
                     Traits=WTF::HashMapValueTraits<CLSIDHashTraits,WTF::HashTraits<COMPtr<IClassFactory>>>,
                     KeyTraits=CLSIDHashTraits,
                     ValueTraits=WTF::HashMapValueTraits<CLSIDHashTraits,WTF::HashTraits<COMPtr<IClassFactory>>>,
                     T=int,
                     Extra=int &
                 ]
                 C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\private\wtf/HashMap.h(386) : see reference to function template instantiation 'WTF::HashTableAddResult<IteratorType> WTF::HashMap<KeyArg,MappedArg,HashArg,KeyTraitsArg>::inlineAdd<const _GUID&,T&>(K,V)' being compiled
                 with
                 [
                     IteratorType=WTF::HashTableIterator<_GUID,WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>>,CLSIDHash,WTF::HashMapValueTraits<CLSIDHashTraits,WTF::HashTraits<COMPtr<IClassFactory>>>,CLSIDHashTraits>,
                     KeyArg=CLSID,
                     MappedArg=COMPtr<IClassFactory>,
                     HashArg=CLSIDHash,
                     KeyTraitsArg=CLSIDHashTraits,
                     T=int,
                     K=const _GUID &,
                     V=int &
                 ]
                 ..\..\win\WebKitCOMAPI.cpp(59) : see reference to function template instantiation 'WTF::HashTableAddResult<IteratorType> WTF::HashMap<KeyArg,MappedArg,HashArg,KeyTraitsArg>::add<int>(const _GUID &,T &&)' being compiled
                 with
                 [
                     IteratorType=WTF::HashTableIterator<_GUID,WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<_GUID,COMPtr<IClassFactory>>>,CLSIDHash,WTF::HashMapValueTraits<CLSIDHashTraits,WTF::HashTraits<COMPtr<IClassFactory>>>,CLSIDHashTraits>,
                     KeyArg=CLSID,
                     MappedArg=COMPtr<IClassFactory>,
                     HashArg=CLSIDHash,
                     KeyTraitsArg=CLSIDHashTraits,
                     T=int
                 ]

[Gustavo Noronha (kov)]

I do not want to sound confrontational, but was there a reason to ignore the several red bubbles and land this anyway?

[Anders Carlsson]

(In reply to comment #29)
> I do not want to sound confrontational, but was there a reason to ignore the several red bubbles and land this anyway?

The turnaround time for getting this working was just too long and I wanted to land it because it’s blocking other changes. I was around in #webkit when i landed it.