[webkit-reviews] review denied: [Bug 120333] Implement ES6 Map object : [Attachment 209944] Has full API spec and correctness

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 120333: Implement ES6 Map object
https://bugs.webkit.org/show_bug.cgi?id=120333

Attachment 209944: Has full API spec and correctness
https://bugs.webkit.org/attachment.cgi?id=209944&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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..