[webkit-reviews] review granted: [Bug 120333] Implement ES6 Map object : [Attachment 210047] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 29 17:37:11 PDT 2013
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>".
More information about the webkit-reviews
mailing list