Bug 120333

Summary: Implement ES6 Map object
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: 18runt88, buildbot, commit-queue, eflews.bot, gtk-ews, gyuyoung.kim, rakuco, rego+ews, rniwa, webkit-ews, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 120503    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Has full API spec and correctness
none
Has full API spec and correctness
none
Patch
none
Patch ggaren: review+

Description Oliver Hunt 2013-08-26 17:01:46 PDT
Implement ES6 Map object
Comment 1 Oliver Hunt 2013-08-26 17:02:37 PDT
Created attachment 209694 [details]
Patch
Comment 2 Oliver Hunt 2013-08-26 17:03:28 PDT
Comment on attachment 209694 [details]
Patch

Not intended for review (O(N) behavior in a "map" is probably unexpected)
Comment 3 Oliver Hunt 2013-08-27 11:30:55 PDT
Created attachment 209787 [details]
Patch
Comment 4 Oliver Hunt 2013-08-27 15:33:05 PDT
Created attachment 209811 [details]
Patch
Comment 5 WebKit Commit Bot 2013-08-27 15:33:58 PDT
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.
Comment 6 Oliver Hunt 2013-08-28 12:18:16 PDT
Created attachment 209916 [details]
Patch
Comment 7 WebKit Commit Bot 2013-08-28 12:20:09 PDT
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 8 Build Bot 2013-08-28 12:36:04 PDT
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 9 Early Warning System Bot 2013-08-28 12:36:56 PDT
Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1605345
Comment 10 Early Warning System Bot 2013-08-28 12:38:48 PDT
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 11 Build Bot 2013-08-28 12:50:11 PDT
Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1625328
Comment 12 Build Bot 2013-08-28 13:02:51 PDT
Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1624340
Comment 13 EFL EWS Bot 2013-08-28 13:50:34 PDT
Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1626332
Comment 14 EFL EWS Bot 2013-08-28 14:06:51 PDT
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 15 kov's GTK+ EWS bot 2013-08-28 15:29:06 PDT
Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1591701
Comment 16 Oliver Hunt 2013-08-28 18:24:02 PDT
Created attachment 209940 [details]
Patch
Comment 17 WebKit Commit Bot 2013-08-28 18:26:34 PDT
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 18 Early Warning System Bot 2013-08-28 18:35:58 PDT
Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1605440
Comment 19 EFL EWS Bot 2013-08-28 18:37:36 PDT
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 20 Early Warning System Bot 2013-08-28 18:39:14 PDT
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 21 Build Bot 2013-08-28 18:47:03 PDT
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 22 EFL EWS Bot 2013-08-28 18:49:15 PDT
Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1627406
Comment 23 Build Bot 2013-08-28 19:03:11 PDT
Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1625426
Comment 24 Build Bot 2013-08-28 19:08:04 PDT
Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1605439
Comment 25 Oliver Hunt 2013-08-28 19:23:57 PDT
Created attachment 209944 [details]
Has full API spec and correctness
Comment 26 WebKit Commit Bot 2013-08-28 19:26:32 PDT
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 27 Early Warning System Bot 2013-08-28 19:32:12 PDT
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 28 EFL EWS Bot 2013-08-28 19:32:50 PDT
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 29 Early Warning System Bot 2013-08-28 19:34:23 PDT
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 30 kov's GTK+ EWS bot 2013-08-28 19:44:46 PDT
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 31 EFL EWS Bot 2013-08-28 19:51:29 PDT
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 32 Build Bot 2013-08-28 19:52:06 PDT
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 33 Build Bot 2013-08-28 20:09:50 PDT
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 34 Sam Weinig 2013-08-28 20:17:41 PDT
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 35 Geoffrey Garen 2013-08-28 20:28:26 PDT
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 36 Darin Adler 2013-08-29 10:23:12 PDT
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.
Comment 37 Oliver Hunt 2013-08-29 10:27:11 PDT
Created attachment 209998 [details]
Has full API spec and correctness
Comment 38 WebKit Commit Bot 2013-08-29 10:28:36 PDT
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 39 Early Warning System Bot 2013-08-29 10:40:36 PDT
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
Comment 40 Oliver Hunt 2013-08-29 10:40:50 PDT
(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 41 Early Warning System Bot 2013-08-29 10:42:31 PDT
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 42 Darin Adler 2013-08-29 10:44:19 PDT
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 43 Build Bot 2013-08-29 10:53:32 PDT
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 44 Build Bot 2013-08-29 10:58:28 PDT
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 45 EFL EWS Bot 2013-08-29 11:18:43 PDT
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 46 EFL EWS Bot 2013-08-29 11:32:05 PDT
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 47 kov's GTK+ EWS bot 2013-08-29 11:58:38 PDT
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 48 Build Bot 2013-08-29 13:35:33 PDT
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
Comment 49 Oliver Hunt 2013-08-29 16:18:26 PDT
Created attachment 210043 [details]
Patch
Comment 50 WebKit Commit Bot 2013-08-29 16:22:18 PDT
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 51 Early Warning System Bot 2013-08-29 16:45:00 PDT
Comment on attachment 210043 [details]
Patch

Attachment 210043 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1623737
Comment 52 Build Bot 2013-08-29 16:47:16 PDT
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
Comment 53 Oliver Hunt 2013-08-29 16:49:55 PDT
Created attachment 210047 [details]
Patch
Comment 54 Geoffrey Garen 2013-08-29 17:36:34 PDT
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 55 Geoffrey Garen 2013-08-29 17:37:56 PDT
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?
Comment 56 Oliver Hunt 2013-08-29 17:54:49 PDT
Committed r154861: <http://trac.webkit.org/changeset/154861>
Comment 57 Oliver Hunt 2013-08-30 09:53:52 PDT
(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()