RESOLVED DUPLICATE of bug 137200 115905
Do not copy the HashMap template for use with RefPtr keys
https://bugs.webkit.org/show_bug.cgi?id=115905
Summary Do not copy the HashMap template for use with RefPtr keys
Mikhail Pozdnyakov
Reported 2013-05-10 06:50:13 PDT
SSIA. At the moment specialization of HashMap for use with RefPtr keys contains the whole copy of generic HashMap class, which is bad.
Attachments
WIP (24.05 KB, patch)
2013-05-10 07:17 PDT, Mikhail Pozdnyakov
no flags
WIP 2 (14.34 KB, patch)
2013-05-12 02:02 PDT, Mikhail Pozdnyakov
no flags
WIP 3 (34.44 KB, patch)
2013-05-13 07:07 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (679.64 KB, application/zip)
2013-05-13 17:57 PDT, Build Bot
no flags
WIP 3 run EWS (34.44 KB, patch)
2013-05-14 00:20 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (545.29 KB, application/zip)
2013-05-14 02:26 PDT, Build Bot
no flags
patch (38.05 KB, patch)
2013-05-14 05:29 PDT, Mikhail Pozdnyakov
no flags
patch v2 (37.64 KB, patch)
2013-05-14 13:29 PDT, Mikhail Pozdnyakov
no flags
patch v3 (37.50 KB, patch)
2013-05-16 05:46 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
patch v4 (34.99 KB, patch)
2013-05-16 06:50 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
try to fix MAC build (35.65 KB, patch)
2013-05-17 01:41 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
fixed MAC build (35.83 KB, patch)
2013-06-17 05:19 PDT, Mikhail Pozdnyakov
no flags
patch v7 (alternate approach) (31.09 KB, patch)
2013-07-12 09:18 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
Mikhail Pozdnyakov
Comment 1 2013-05-10 07:17:08 PDT
Mikhail Pozdnyakov
Comment 2 2013-05-10 07:34:27 PDT
Comment on attachment 201343 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=201343&action=review > Source/WTF/wtf/RefPtrHashMap.h:151 > + inline void HashMapBase<RefPtr<T>, U, V, W, X>::remove(RawKeyType key) argh! it will be probably hidden with HashMap::remove(iterator it)..
Darin Adler
Comment 3 2013-05-10 17:51:49 PDT
(In reply to comment #2) > > Source/WTF/wtf/RefPtrHashMap.h:151 > > + inline void HashMapBase<RefPtr<T>, U, V, W, X>::remove(RawKeyType key) > > argh! it will be probably hidden with HashMap::remove(iterator it).. You should be able to fix that with "using".
Darin Adler
Comment 4 2013-05-10 17:52:17 PDT
(In reply to comment #3) > (In reply to comment #2) > > > Source/WTF/wtf/RefPtrHashMap.h:151 > > > + inline void HashMapBase<RefPtr<T>, U, V, W, X>::remove(RawKeyType key) > > > > argh! it will be probably hidden with HashMap::remove(iterator it).. > > You should be able to fix that with "using". And I think there’s even a way to delete it in C++11 compilers.
Mikhail Pozdnyakov
Comment 5 2013-05-12 02:02:01 PDT
Created attachment 201483 [details] WIP 2 Another approach, the specialization is inherited from generic HashMap, providing original traits templates arguments.
Darin Adler
Comment 6 2013-05-12 22:06:21 PDT
I like the idea of eliminating this silly duplication. I think I liked your first attempt better than the second, not sure. I’m hoping we can use this for other types, such as RetainPtr, once we make it more rational.
Mikhail Pozdnyakov
Comment 7 2013-05-13 07:07:03 PDT
Created attachment 201560 [details] WIP 3 Combination of 1st and 2nd approaches, also made it re-usable for other key types, like RetainPtr.
WebKit Commit Bot
Comment 8 2013-05-13 07:09:30 PDT
Attachment 201560 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/HashMap.h', u'Source/WTF/wtf/HashTraits.h', u'Source/WTF/wtf/RefPtrHashMap.h']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 9 2013-05-13 17:57:35 PDT
Comment on attachment 201560 [details] WIP 3 Attachment 201560 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/346957 New failing tests: http/tests/security/same-origin-document-domain-storage-allowed.html fast/loader/stateobjects/replacestate-in-iframe.html storage/domstorage/sessionstorage/window-open.html
Build Bot
Comment 10 2013-05-13 17:57:38 PDT
Created attachment 201655 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Mikhail Pozdnyakov
Comment 11 2013-05-14 00:19:38 PDT
(In reply to comment #9) > (From update of attachment 201560 [details]) > Attachment 201560 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/346957 > > New failing tests: > http/tests/security/same-origin-document-domain-storage-allowed.html > fast/loader/stateobjects/replacestate-in-iframe.html > storage/domstorage/sessionstorage/window-open.html that looks strange..
Mikhail Pozdnyakov
Comment 12 2013-05-14 00:20:10 PDT
Created attachment 201684 [details] WIP 3 run EWS
WebKit Commit Bot
Comment 13 2013-05-14 00:21:45 PDT
Attachment 201684 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/HashMap.h', u'Source/WTF/wtf/HashTraits.h', u'Source/WTF/wtf/RefPtrHashMap.h']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 14 2013-05-14 02:26:16 PDT
Comment on attachment 201684 [details] WIP 3 run EWS Attachment 201684 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/477020 New failing tests: http/tests/security/same-origin-document-domain-storage-allowed.html fast/loader/stateobjects/replacestate-in-iframe.html storage/domstorage/sessionstorage/window-open.html
Build Bot
Comment 15 2013-05-14 02:26:19 PDT
Created attachment 201692 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Mikhail Pozdnyakov
Comment 16 2013-05-14 04:39:02 PDT
(In reply to comment #15) > Created an attachment (id=201692) [details] > Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2 ok, ok.. found a stupid mistake in my patch: AddResult add(const KeyType& key, MappedPassInType mapped) { return BaseType::set(key, mapped); } -> AddResult add(const KeyType& key, MappedPassInType mapped) { return BaseType::add(key, mapped); }
Mikhail Pozdnyakov
Comment 17 2013-05-14 05:29:15 PDT
Created attachment 201702 [details] patch Fixed couple of stupid mistakes (see comment above), re-arranged methods definitions accordingly to their declaration order, added change log.
WebKit Commit Bot
Comment 18 2013-05-14 05:30:49 PDT
Attachment 201702 [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/HashTraits.h', u'Source/WTF/wtf/RefPtrHashMap.h']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 19 2013-05-14 10:02:39 PDT
Comment on attachment 201702 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=201702&action=review I worry that these make it a harder to see what set of functions is supported by HashMap. > Source/WTF/ChangeLog:61 > + * wtf/HashMap.h: > + (WTF): > + (HashMap): > + (WTF::HashMap::keys): > + (WTF::HashMap::values): > + (WTF::HashMap::impl): > + (WTF::::find): > + (WTF::::contains): > + (WTF::::get): > + (WTF::::set): > + (WTF::::add): > + (WTF::::remove): > + (WTF::::take): > + (WTF::::inlineAdd): > + (WTF::::swap): > + (WTF::::size): > + (WTF::::capacity): > + (WTF::::isEmpty): > + (WTF::::begin): > + (WTF::::end): > + (WTF::::clear): > + (WTF::::checkConsistency): > + * wtf/HashTraits.h: > + (GenericHashTraits): > + * wtf/RefPtrHashMap.h: > + (WTF): > + (WTF::::find): > + (WTF::::contains): > + (WTF::::inlineAdd): > + (WTF::::set): > + (WTF::::add): > + (WTF::::inlineGet): > + (WTF::::get): > + (WTF::::remove): > + (WTF::::take): The prepare-ChangeLog script completely screwed this up. Please edit by hand so that it’s a useful list of functions, or delete the functions entirely. Either would be better than this near-gibberish the script produced. > Source/WTF/wtf/HashMap.h:128 > + private: > + typedef HashMapBase<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg, KeyTraitsArg::traitTypesCount> BaseType; > + typedef typename BaseType::KeyTraits KeyTraits; > + typedef typename BaseType::MappedTraits MappedTraits; > + typedef typename BaseType::ValueTraits ValueTraits; Are all these typedefs needed? We should omit any that are not needed. > Source/WTF/wtf/HashMap.h:133 > + typedef typename BaseType::KeyType KeyType; > + typedef typename BaseType::MappedType MappedType; > + typedef typename BaseType::ValueType ValueType; Are these typedefs needed? If these types are public and in the base class, why do we need to re-typedef them? Could we omit them or make use of “using” instead? > Source/WTF/wtf/HashMap.h:145 > + private: > + typedef typename BaseType::MappedPassInType MappedPassInType; > + typedef typename BaseType::MappedPassOutType MappedPassOutType; > + typedef typename BaseType::MappedPeekType MappedPeekType; > + typedef typename BaseType::MappedPassInReferenceType MappedPassInReferenceType; > + > + typedef typename BaseType::HashFunctions HashFunctions; > + typedef typename BaseType::HashTableType HashTableType; > + > + class HashMapKeysProxy; > + class HashMapValuesProxy; Are all these typedefs and declarations needed? We should omit any that are not needed. > Source/WTF/wtf/HashMap.h:150 > + typedef typename BaseType::iterator iterator; > + typedef typename BaseType::const_iterator const_iterator; > + typedef typename BaseType::AddResult AddResult; Are these typedefs needed? If these types are public and in the base class, why do we need to re-typedef them? Could we omit them or make use of “using” instead? > Source/WTF/wtf/HashMap.h:169 > + HashMapKeysProxy& keys() { return static_cast<HashMapKeysProxy&>(*this); } > + const HashMapKeysProxy& keys() const { return static_cast<const HashMapKeysProxy&>(*this); } > + > + HashMapValuesProxy& values() { return static_cast<HashMapValuesProxy&>(*this); } > + const HashMapValuesProxy& values() const { return static_cast<const HashMapValuesProxy&>(*this); } Why is a cast needed? Won’t these all work without an explicit static_cast?
Mikhail Pozdnyakov
Comment 20 2013-05-14 11:57:33 PDT
(In reply to comment #19) > (From update of attachment 201702 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201702&action=review > > I worry that these make it a harder to see what set of functions is supported by HashMap. yes, but we gain some flexibility in the class interface.. we have already examples in WTF when the User needs to look at both base and child class in order to see the whole picture (for instance RefCounted, RefCountedBase). Moreover the current situation with having HashMap<RefPtr<>, > specialization as a separate class does not make life any better. > > > Source/WTF/ChangeLog:61 > > + * wtf/HashMap.h: > > + (WTF): > > + (HashMap): > > + (WTF::HashMap::keys): > > + (WTF::HashMap::values): > > + (WTF::HashMap::impl): > > + (WTF::::find): > > + (WTF::::contains): > > + (WTF::::get): > > + (WTF::::set): > > + (WTF::::add): > > + (WTF::::remove): > > + (WTF::::take): > > + (WTF::::inlineAdd): > > + (WTF::::swap): > > + (WTF::::size): > > + (WTF::::capacity): > > + (WTF::::isEmpty): > > + (WTF::::begin): > > + (WTF::::end): > > + (WTF::::clear): > > + (WTF::::checkConsistency): > > + * wtf/HashTraits.h: > > + (GenericHashTraits): > > + * wtf/RefPtrHashMap.h: > > + (WTF): > > + (WTF::::find): > > + (WTF::::contains): > > + (WTF::::inlineAdd): > > + (WTF::::set): > > + (WTF::::add): > > + (WTF::::inlineGet): > > + (WTF::::get): > > + (WTF::::remove): > > + (WTF::::take): > > The prepare-ChangeLog script completely screwed this up. Please edit by hand so that it’s a useful list of functions, or delete the functions entirely. Either would be better than this near-gibberish the script produced. > Sure thing. > > Source/WTF/wtf/HashMap.h:128 > > + private: > > + typedef HashMapBase<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg, KeyTraitsArg::traitTypesCount> BaseType; > > + typedef typename BaseType::KeyTraits KeyTraits; > > + typedef typename BaseType::MappedTraits MappedTraits; > > + typedef typename BaseType::ValueTraits ValueTraits; > > Are all these typedefs needed? We should omit any that are not needed. I guess some of them can be omitted indeed. > > > Source/WTF/wtf/HashMap.h:133 > > + typedef typename BaseType::KeyType KeyType; > > + typedef typename BaseType::MappedType MappedType; > > + typedef typename BaseType::ValueType ValueType; > > Are these typedefs needed? If these types are public and in the base class, why do we need to re-typedef them? For convenience, otherwise we'd need something like BaseType::KeyType in every place where they are used. Could we omit them or make use of “using” instead? mm.. would "using" give a more benefit here comparing to "typedef"? As far as I know "using" has the same semantics as if it were introduced by the typedef specifier, and the benefit of "using" is that it makes possible to "typedef" template classes which however is not the case here. > > > Source/WTF/wtf/HashMap.h:145 > > + private: > > + typedef typename BaseType::MappedPassInType MappedPassInType; > > + typedef typename BaseType::MappedPassOutType MappedPassOutType; > > + typedef typename BaseType::MappedPeekType MappedPeekType; > > + typedef typename BaseType::MappedPassInReferenceType MappedPassInReferenceType; > > + > > + typedef typename BaseType::HashFunctions HashFunctions; > > + typedef typename BaseType::HashTableType HashTableType; > > + > > + class HashMapKeysProxy; > > + class HashMapValuesProxy; > > Are all these typedefs and declarations needed? We should omit any that are not needed. > > > Source/WTF/wtf/HashMap.h:150 > > + typedef typename BaseType::iterator iterator; > > + typedef typename BaseType::const_iterator const_iterator; > > + typedef typename BaseType::AddResult AddResult; > > Are these typedefs needed? If these types are public and in the base class, why do we need to re-typedef them? Could we omit them or make use of “using” instead? > > > Source/WTF/wtf/HashMap.h:169 > > + HashMapKeysProxy& keys() { return static_cast<HashMapKeysProxy&>(*this); } > > + const HashMapKeysProxy& keys() const { return static_cast<const HashMapKeysProxy&>(*this); } > > + > > + HashMapValuesProxy& values() { return static_cast<HashMapValuesProxy&>(*this); } > > + const HashMapValuesProxy& values() const { return static_cast<const HashMapValuesProxy&>(*this); } > > Why is a cast needed? Won’t these all work without an explicit static_cast? That had been already there, and the cast is needed I guess as those classes are just inherited from HashMap, so the pointer to base class needs cast to be treated as a pointer to child class.
Mikhail Pozdnyakov
Comment 21 2013-05-14 13:29:42 PDT
Created attachment 201752 [details] patch v2 Fixed change log, removed "typedefs" of types which are not used frequently within their classes.
WebKit Commit Bot
Comment 22 2013-05-14 13:31:07 PDT
Attachment 201752 [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/HashTraits.h', u'Source/WTF/wtf/RefPtrHashMap.h']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mikhail Pozdnyakov
Comment 23 2013-05-15 04:43:03 PDT
(In reply to comment #20) > (In reply to comment #19) > > (From update of attachment 201702 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=201702&action=review > > > > I worry that these make it a harder to see what set of functions is supported by HashMap. > yes, but we gain some flexibility in the class interface.. > we have already examples in WTF when the User needs to look at both base and child class in order to see the whole picture (for instance RefCounted, RefCountedBase). > Moreover the current situation with having HashMap<RefPtr<>, > specialization > as a separate class does not make life any better. > Think it's worth adding that the proposed approach can be quite easily applied to various cases when a supplementary key type would make sense, HashMap<RetainPtr, > was already mentioned, but this also can be used with HashMap<String, ..> (having const char* as another key type) and probably others.
Mikhail Pozdnyakov
Comment 24 2013-05-16 05:46:31 PDT
Created attachment 201947 [details] patch v3 Now HashMap is not split to HashMapBase and HashMap, so that it is easy to see all the existing methods.
WebKit Commit Bot
Comment 25 2013-05-16 05:48:11 PDT
Attachment 201947 [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/RefPtrHashMap.h']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:26: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WTF/wtf/HashMap.h:47: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 26 2013-05-16 06:39:47 PDT
Comment on attachment 201947 [details] patch v3 Attachment 201947 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/470927
Mikhail Pozdnyakov
Comment 27 2013-05-16 06:50:10 PDT
Created attachment 201950 [details] patch v4 Arranged HashMap method definitions in a correct order.
WebKit Commit Bot
Comment 28 2013-05-16 06:51:19 PDT
Attachment 201950 [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/RefPtrHashMap.h']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:26: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WTF/wtf/HashMap.h:47: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 29 2013-05-16 07:35:37 PDT
Comment on attachment 201950 [details] patch v4 Attachment 201950 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/487006
Mikhail Pozdnyakov
Comment 30 2013-05-16 15:49:33 PDT
(In reply to comment #29) > (From update of attachment 201950 [details]) > Attachment 201950 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/487006 Should tweak exported symbols for MAC apparently.. but still it would be nice to get reviewers feedback on the approach.
Build Bot
Comment 31 2013-05-16 19:29:33 PDT
Mikhail Pozdnyakov
Comment 32 2013-05-17 01:41:53 PDT
Created attachment 202050 [details] try to fix MAC build
WebKit Commit Bot
Comment 33 2013-05-17 01:44:03 PDT
Attachment 202050 [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/RefPtrHashMap.h', u'Source/WebCore/WebCore.exp.in']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:26: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WTF/wtf/HashMap.h:47: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 34 2013-05-17 02:16:24 PDT
Comment on attachment 202050 [details] try to fix MAC build Attachment 202050 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/489232
Build Bot
Comment 35 2013-05-17 02:45:47 PDT
Comment on attachment 202050 [details] try to fix MAC build Attachment 202050 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/492233
Build Bot
Comment 36 2013-05-17 03:46:47 PDT
Comment on attachment 202050 [details] try to fix MAC build Attachment 202050 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/488284
Mikhail Pozdnyakov
Comment 37 2013-05-17 13:13:17 PDT
Darin, could you please give your opinion on the latest approach?
Darin Adler
Comment 38 2013-05-19 04:46:08 PDT
I think the approach is OK. It hadn’t occurred to me that the main feature of the RefPtr hash map is built-in support for an additional key type, raw pointers. I think the followup question is what other key types might lead us to want support for additional key types. We talked about RetainPtr, but another obvious example is String and const char*. HashSet’s approach to this is to allow explicit use of a hash translator for the find, contains, and add functions. Not supporting a hash translator for the remove function is a strange omission in HashSet, but I suppose we haven’t needed it yet. The approach in RefPtr HashMap to automatically overload and translate raw pointers is more convenient than the hash translator approach, but less flexible since it only works for a single type. It’s also strange that a RefPtr HashSet requires a hash translator to work with raw pointers. I’d like to see more unity between HashSet and HashMap on this. It might be cleaner if we could make it possible to use a translator with HashMap, and to have automatic support for additional key types be a feature of HashSet. On the other hand, bloating both these class templates with these new features might be a mistake. Another area we need to work on is to cut down on code size when these class templates are used. I think some of the Blink folks are interested in this because they did not like the fix I did for bug 115807 and would prefer that the code size be smaller for typical HashMap use.
Mikhail Pozdnyakov
Comment 39 2013-06-17 05:12:21 PDT
(In reply to comment #38) > I think the approach is OK. It hadn’t occurred to me that the main feature of the RefPtr hash map is built-in support for an additional key type, raw pointers. > > I think the followup question is what other key types might lead us to want support for additional key types. We talked about RetainPtr, but another obvious example is String and const char*. > > HashSet’s approach to this is to allow explicit use of a hash translator for the find, contains, and add functions. Not supporting a hash translator for the remove function is a strange omission in HashSet, but I suppose we haven’t needed it yet. > > The approach in RefPtr HashMap to automatically overload and translate raw pointers is more convenient than the hash translator approach, but less flexible since it only works for a single type. It’s also strange that a RefPtr HashSet requires a hash translator to work with raw pointers. > > I’d like to see more unity between HashSet and HashMap on this. It might be cleaner if we could make it possible to use a translator with HashMap, and to have automatic support for additional key types be a feature of HashSet. > > On the other hand, bloating both these class templates with these new features might be a mistake. > > Another area we need to work on is to cut down on code size when these class templates are used. I think some of the Blink folks are interested in this because they did not like the fix I did for bug 115807 and would prefer that the code size be smaller for typical HashMap use. I could spread the proposed approach to HashSet as well, I also think it is more convenient to use than putting translator on caller side. As for flexibility, it is possible to create specializations for ExtraKeysCountArg==2 or 3 if needed.
Mikhail Pozdnyakov
Comment 40 2013-06-17 05:19:50 PDT
Created attachment 204810 [details] fixed MAC build
WebKit Commit Bot
Comment 41 2013-06-17 05:21:29 PDT
Attachment 204810 [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/RefPtrHashMap.h', u'Source/WebCore/WebCore.exp.in']" exit_code: 1 Source/WTF/wtf/RefPtrHashMap.h:26: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WTF/wtf/HashMap.h:47: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mikhail Pozdnyakov
Comment 42 2013-07-12 09:18:14 PDT
Created attachment 206550 [details] patch v7 (alternate approach) Added alternate versions of HasMap interface methods which do not enforce converting key arguments to Key type but instead let HasMap's own HashTranslator to deal with them. Using of these functions do not usually require any extra effort on client side (e.g., explicit passing of used HashTranslator). Using of these methods is benefitial if Key type has some relative types that could be handled by HasMap's HashFunctions - objects of these types can be used as the key arguments, for example smart pointers and raw pointers of the same type or various string classes. This approach is also much simpler than all the previous ones.
Build Bot
Comment 43 2013-07-12 09:55:28 PDT
Comment on attachment 206550 [details] patch v7 (alternate approach) Attachment 206550 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1066062
Build Bot
Comment 44 2013-07-12 10:18:09 PDT
Comment on attachment 206550 [details] patch v7 (alternate approach) Attachment 206550 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1066065
Anders Carlsson
Comment 45 2014-12-21 10:22:20 PST
Comment on attachment 206550 [details] patch v7 (alternate approach) Sam did this some time ago.
Darin Adler
Comment 46 2014-12-21 17:28:58 PST
He used bug 137200 to do it. *** This bug has been marked as a duplicate of bug 137200 ***
Note You need to log in before you can comment on or make changes to this bug.