Implement ES6 Map object
Created attachment 209694 [details] Patch
Comment on attachment 209694 [details] Patch Not intended for review (O(N) behavior in a "map" is probably unexpected)
Created attachment 209787 [details] Patch
Created attachment 209811 [details] Patch
Attachment 209811 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1 Source/JavaScriptCore/runtime/MapPrototype.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:28: You should add a blank line after implementation file's own header. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:37: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/runtime/MapData.h:76: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 209916 [details] Patch
Attachment 209916 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1 Source/JavaScriptCore/runtime/MapPrototype.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.cpp:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:39: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/runtime/MapConstructor.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:30: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:83: Missing space inside { }. [whitespace/braces] [5] Source/JavaScriptCore/runtime/MapData.h:115: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:115: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:116: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:117: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:136: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 14 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1625327
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1605345
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1625330
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1625328
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1624340
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1626332
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1627345
Comment on attachment 209916 [details] Patch Attachment 209916 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1591701
Created attachment 209940 [details] Patch
Attachment 209940 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1 Source/JavaScriptCore/runtime/MapPrototype.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapPrototype.cpp:111: One line control clauses should not use braces. [whitespace/braces] [4] Source/JavaScriptCore/runtime/MapData.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.cpp:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:31: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:40: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/runtime/MapConstructor.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:30: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:114: Missing space inside { }. [whitespace/braces] [5] Source/JavaScriptCore/runtime/MapData.h:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/JavaScriptCore/runtime/MapData.h:158: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:158: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:159: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:160: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:205: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 16 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209940 [details] Patch Attachment 209940 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1605440
Comment on attachment 209940 [details] Patch Attachment 209940 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1618459
Comment on attachment 209940 [details] Patch Attachment 209940 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1618460
Comment on attachment 209940 [details] Patch Attachment 209940 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1631028
Comment on attachment 209940 [details] Patch Attachment 209940 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1627406
Comment on attachment 209940 [details] Patch Attachment 209940 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1625426
Comment on attachment 209940 [details] Patch Attachment 209940 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1605439
Created attachment 209944 [details] Has full API spec and correctness
Attachment 209944 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1 Source/JavaScriptCore/runtime/MapPrototype.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.cpp:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:31: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:40: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/runtime/MapConstructor.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:30: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:114: Missing space inside { }. [whitespace/braces] [5] Source/JavaScriptCore/runtime/MapData.h:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/JavaScriptCore/runtime/MapData.h:158: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:158: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:159: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:160: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:205: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 15 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209944 [details] Has full API spec and correctness Attachment 209944 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1626437
Comment on attachment 209944 [details] Has full API spec and correctness Attachment 209944 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1638040
Comment on attachment 209944 [details] Has full API spec and correctness Attachment 209944 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1625441
Comment on attachment 209944 [details] Has full API spec and correctness Attachment 209944 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1625444
Comment on attachment 209944 [details] Has full API spec and correctness Attachment 209944 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1624453
Comment on attachment 209944 [details] Has full API spec and correctness Attachment 209944 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1635038
Comment on attachment 209944 [details] Has full API spec and correctness Attachment 209944 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1639044
Comment on attachment 209944 [details] Has full API spec and correctness View in context: https://bugs.webkit.org/attachment.cgi?id=209944&action=review > Source/JavaScriptCore/runtime/MapConstructor.cpp:56 > + return JSValue::encode(throwTypeError(exec, ASCIILiteral("Map constructor does not accept arguments"))); > + if (!exec->argument(1).isUndefined()) > + return JSValue::encode(throwError(exec, createRangeError(exec, WTF::ASCIILiteral("Invalid comparator function")))); > + On one line you use ASCIILiteral, the other WTF::ASCIILiteral! Make up your mind! > Source/JavaScriptCore/runtime/MapConstructor.h:39 > + static MapConstructor* create(ExecState* exec, JSGlobalObject* globalObject, Structure* structure, MapPrototype* functionPrototype) This could probably take a VM& rather than an ExecState*. > Source/JavaScriptCore/runtime/MapPrototype.cpp:65 > + JSC_NATIVE_FUNCTION(vm.propertyNames->clear, mapProtoFuncClear, DontEnum, 0); > + JSC_NATIVE_FUNCTION(vm.propertyNames->deleteKeyword, mapProtoFuncDelete, DontEnum, 1); > + JSC_NATIVE_FUNCTION(vm.propertyNames->forEach, mapProtoFuncForEach, DontEnum, 1); > + JSC_NATIVE_FUNCTION(vm.propertyNames->get, mapProtoFuncGet, DontEnum, 1); > + JSC_NATIVE_FUNCTION(vm.propertyNames->has, mapProtoFuncHas, DontEnum, 1); > + JSC_NATIVE_FUNCTION(vm.propertyNames->set, mapProtoFuncSet, DontEnum, 2); Why set them directly rather than use a lut?
Comment on attachment 209944 [details] Has full API spec and correctness View in context: https://bugs.webkit.org/attachment.cgi?id=209944&action=review Looks good. I have a few comments below, and it looks like this patch isn't quite building yet. > Source/JavaScriptCore/ChangeLog:45 > + * JavaScriptCore.xcodeproj/project.pbxproj: > + * runtime/CommonIdentifiers.h: > + * runtime/JSGlobalObject.cpp: > + (JSC::JSGlobalObject::reset): > + * runtime/JSGlobalObject.h: > + (JSC::JSGlobalObject::mapStructure): > + * runtime/MapConstructor.cpp: Added. > + (JSC::MapConstructor::finishCreation): > + (JSC::constructMap): > + (JSC::MapConstructor::getConstructData): > + (JSC::MapConstructor::getCallData): > + * runtime/MapConstructor.h: Added. > + (JSC::MapConstructor::create): > + (JSC::MapConstructor::createStructure): > + (JSC::MapConstructor::MapConstructor): > + * runtime/MapData.cpp: Added. > + (JSC::MapData::ensureSpaceForAppend): > + (JSC::MapData::find): > + (JSC::MapData::set): > + (JSC::MapData::get): > + (JSC::MapData::remove): > + (JSC::MapData::visitChildren): > + * runtime/MapData.h: Added. > + (JSC::MapData::create): > + (JSC::MapData::createStructure): > + (JSC::MapData::contains): > + (JSC::MapData::size): > + (JSC::MapData::clear): > + (JSC::MapData::MapData): > + * runtime/MapInstance.cpp: Added. > + (JSC::MapInstance::visitChildren): > + * runtime/MapInstance.h: Added. > + (JSC::MapInstance::createStructure): > + (JSC::MapInstance::create): > + (JSC::MapInstance::mapData): > + (JSC::MapInstance::MapInstance): If you're not going to explain what you did, I don't think this listing is useful. So, please remove this listing, or explain what you did. > Source/JavaScriptCore/runtime/MapConstructor.cpp:28 > +#include "config.h" > + > +#include "MapConstructor.h" These lines should go together. > Source/JavaScriptCore/runtime/MapConstructor.cpp:51 > + // any arguments that could result in us throw. "that could result in us throw"? >> Source/JavaScriptCore/runtime/MapData.cpp:28 >> +#include "config.h" >> + >> +#include "JSCJSValueInlines.h" > > Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] These lines should go together. >> Source/JavaScriptCore/runtime/MapData.cpp:30 >> +#include "MapData.h" > > Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Ditto. > Source/JavaScriptCore/runtime/MapData.cpp:139 > +template <MapData::CopyMode mode> void MapData::copyAndPackIfPossible(Entry* destination, size_t newSize) I think this would be clearer as two different functions rather than one function with two templatized modes. Very little code here is in common. > Source/JavaScriptCore/runtime/MapData.cpp:148 > + memcpy(destination + newEnd, &entry, sizeof(Entry)); Let's use a copy constructor here instead. > Source/JavaScriptCore/runtime/MapData.cpp:184 > +void MapData::visitChildren(JSCell* cell, SlotVisitor& visitor) I don't think your test will trigger this function. Can you make it more aggressive, to cover this? > Source/JavaScriptCore/runtime/MapData.h:106 > + void next() > + { > + ASSERT(!atEnd()); > + Entry* entries = m_mapData->m_entries; > + size_t index = m_index + 1; > + size_t end = m_mapData->m_size; > + while (index < end && !entries[index].key()) > + index++; > + m_index = index; > + } Can you add a test for iterating while inserting and removing? > Source/JavaScriptCore/runtime/MapData.h:116 > + ALWAYS_INLINE KeyType(JSValue v) > + { It looks like this function tries to do some sort of double vs int conversion, but it's not clear what the goal is. I think you should factor out the conversion into a function, and name the function to indicate what we're trying to do. > Source/JavaScriptCore/runtime/MapData.h:215 > + HashMap<KeyType, size_t, KeyHash, KeyTraits, IndexTraits> m_valueKeyedTable; > + HashMap<StringImpl*, size_t> m_stringKeyedTable; JavaScript never runs out of humor value. > Source/JavaScriptCore/runtime/MapInstance.h:35 > +class MapInstance : public JSNonFinalObject { Let's call this "JSMap" to match our other terminology..
Comment on attachment 209944 [details] Has full API spec and correctness View in context: https://bugs.webkit.org/attachment.cgi?id=209944&action=review >> Source/JavaScriptCore/ChangeLog:45 >> + (JSC::MapInstance::MapInstance): > > If you're not going to explain what you did, I don't think this listing is useful. So, please remove this listing, or explain what you did. I normally don’t include the list of newly added functions when I add a file even though the script generates the list. I haven’t heard a good argument against my practice.
Created attachment 209998 [details] Has full API spec and correctness
Attachment 209998 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1 Source/JavaScriptCore/runtime/MapPrototype.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.cpp:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:31: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/JavaScriptCore/runtime/MapInstance.cpp:40: Missing space after , [whitespace/comma] [3] Source/JavaScriptCore/runtime/MapConstructor.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:30: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/MapData.h:114: Missing space inside { }. [whitespace/braces] [5] Source/JavaScriptCore/runtime/MapData.h:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/JavaScriptCore/runtime/MapData.h:158: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:158: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:159: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:160: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/MapData.h:205: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 15 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1643028
(In reply to comment #36) > (From update of attachment 209944 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209944&action=review > > >> Source/JavaScriptCore/ChangeLog:45 > >> + (JSC::MapInstance::MapInstance): > > > > If you're not going to explain what you did, I don't think this listing is useful. So, please remove this listing, or explain what you did. > > I normally don’t include the list of newly added functions when I add a file even though the script generates the list. I haven’t heard a good argument against my practice. The changelog is just there so webkit-patch works automatically :D
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1635262
Comment on attachment 209998 [details] Has full API spec and correctness View in context: https://bugs.webkit.org/attachment.cgi?id=209998&action=review >> Source/JavaScriptCore/runtime/MapData.cpp:30 >> +#include "MapData.h" > > Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] I agree with the style checker. This should not have to be included after the Inlines header. > Source/JavaScriptCore/runtime/MapData.cpp:44 > +enum { MinimumMapSize = 16 }; Where does that constant come from? Why an enum instead of a const? This is C++, not C. > Source/JavaScriptCore/runtime/MapData.cpp:58 > + size_t location = 0; This "= 0" is pointless and should be omitted. > Source/JavaScriptCore/runtime/MapData.cpp:60 > + String string = asString(key.value)->value(callFrame); Not sure we need the local variable for this. Could do it all on one line. > Source/JavaScriptCore/runtime/MapData.cpp:75 > + return !find(callFrame, key).isEmpty(); isEmpty? Really? Not typically how iterators work. It would normally be == end(). This also seems like a function that would be inlined. > Source/JavaScriptCore/runtime/MapData.cpp:87 > + ASSERT(map.contains(key)); Seems a little strange to assert this. Do we really not trust map.add? > Source/JavaScriptCore/runtime/MapData.cpp:97 > + if (!location) > + return; Usually we use == end, in contains we use isEmpty, and here we use ! -- would be better to be consistent. > Source/JavaScriptCore/runtime/MapData.cpp:106 > + String string = asString(key.value)->value(callFrame); Not sure we need the local for this. Might be readable all on one line. > Source/JavaScriptCore/runtime/MapData.cpp:119 > + size_t location = 0; Don't need the "=0" here. > Source/JavaScriptCore/runtime/MapData.cpp:121 > + String string = asString(key.value)->value(callFrame); Not sure we need the local variable for this. Could do it all on one line. > Source/JavaScriptCore/runtime/MapData.cpp:200 > + visitor.appendValues(&entries[0].keyForGC(), size * (sizeof(Entry) / sizeof(WriteBarrier<Unknown>))); Ugly to rely on the layout of the Entry in this subtle way here. If we are going to use it like this, I think it should be a struct and the restriction on its layout should be mentioned in the header. We should also have a compile-time assertion that there is no padding. The getters aren’t adding value if they just return references to same named data members. > Source/JavaScriptCore/runtime/MapData.cpp:209 > + if (token == MapBackingStoreCopyToken > + && visitor.checkIfShouldCopy(thisObject->m_entries)) { Would read better on a single line. > Source/JavaScriptCore/runtime/MapData.cpp:213 > + void* newVector = visitor.allocateNewSpace(thisObject->m_capacity * sizeof(Entry)); > + Entry* newEntries = static_cast<Entry*>(newVector); newVector local is not doing us any good here > Source/JavaScriptCore/runtime/MapData.h:37 > +namespace JSC { > +class MapData : public JSDestructibleObject { Missing blank line. > Source/JavaScriptCore/runtime/MapData.h:38 > +private: Should put public stuff first. Don’t have to define things before they are used. > Source/JavaScriptCore/runtime/MapData.h:174 > + void clear() > + { > + m_valueKeyedTable.clear(); > + m_stringKeyedTable.clear(); > + m_capacity = 0; > + m_size = 0; > + m_deletedCount = 0; > + m_entries = 0; > + } All these functions inlined within the class make the class too hard to read. I suggest putting more of the non-trivial inline functions separate after the class so the class itself is more readable.
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1640222
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1626640
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1591953
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1626647
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1643030
Comment on attachment 209998 [details] Has full API spec and correctness Attachment 209998 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1639299
Created attachment 210043 [details] Patch
Attachment 210043 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/basic-map-expected.txt', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSMap.cpp', u'Source/JavaScriptCore/runtime/JSMap.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1 Source/JavaScriptCore/runtime/MapData.h:173: MapData::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 210043 [details] Patch Attachment 210043 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1623737
Comment on attachment 210043 [details] Patch Attachment 210043 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1639343
Created attachment 210047 [details] Patch
Comment on attachment 210047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210047&action=review r=me, with some more suggestions below. > Source/JavaScriptCore/runtime/JSMap.cpp:28 > +#include "config.h" > + > +#include "JSMap.h" No newline between these, please. > Source/JavaScriptCore/runtime/MapData.cpp:28 > +#include "config.h" > + > +#include "MapData.h" Ditto. > Source/JavaScriptCore/runtime/MapData.cpp:43 > +static const int32_t MinimumMapSize = 8; Should be lower case. > Source/JavaScriptCore/runtime/MapData.cpp:68 > +bool MapData::contains(CallFrame* callFrame, KeyType key) Newline here please. > Source/JavaScriptCore/runtime/MapData.cpp:144 > + entry.value.setWithoutWriteBarrier(jsNumber(newEnd)); I think it's worth a comment here to explain that you're being sneaky here. How about this: "We store overwrite the old entry with a forwarding index for the new entry, so we can fix up our hash tables below without doing additional hash lookups." > Source/JavaScriptCore/runtime/MapData.h:99 > + // Our marking functions expect Entry to maintain this layout You can be a little more specific and say "depend on all of our data members being WriteBarrier<Unknown>".
Comment on attachment 210047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210047&action=review > Source/JavaScriptCore/runtime/MapData.h:59 > + // This is a bit gnarly. We use an index of -1 to indicate the > + // "end()" iterator. By casting to unsigned we can immediately > + // test if both iterators are at the end of their iteration. > + // We need this in order to keep the common case (eg. iter != end()) > + // fast. > + bool atEnd() const { return static_cast<size_t>(m_index) >= static_cast<size_t>(m_mapData->m_size); } I also think this is a bit gnarly. The typical solution, which WTF hash tables use, is to use m_mapData->m_size + 1 as the end index. That works naturally for ++, and avoids special-case checking for atEnd(). Would that work here?
Committed r154861: <http://trac.webkit.org/changeset/154861>
(In reply to comment #55) > (From update of attachment 210047 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210047&action=review > > > Source/JavaScriptCore/runtime/MapData.h:59 > > + // This is a bit gnarly. We use an index of -1 to indicate the > > + // "end()" iterator. By casting to unsigned we can immediately > > + // test if both iterators are at the end of their iteration. > > + // We need this in order to keep the common case (eg. iter != end()) > > + // fast. > > + bool atEnd() const { return static_cast<size_t>(m_index) >= static_cast<size_t>(m_mapData->m_size); } > > I also think this is a bit gnarly. The typical solution, which WTF hash tables use, is to use m_mapData->m_size + 1 as the end index. That works naturally for ++, and avoids special-case checking for atEnd(). Would that work here? I considered that, but decided against it as it introduces a hazard if you use the idiom foo = map->end(); for (ptr = begin(); ptr != end; ++ptr) As Map iteration is specified as being safe under mutation, and has to visit newly added entries - any value that end() stores has to always be larger than size. I guess we could just limit the map size to ~2gig - 1 entry and change the end marker to be <int>::max()