[webkit-reviews] review granted: [Bug 120333] Implement ES6 Map object : [Attachment 210047] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted 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 210047: Patch
https://bugs.webkit.org/attachment.cgi?id=210047&action=review

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