[webkit-reviews] review granted: [Bug 20645] Duplicated constants : [Attachment 23630] Updating the patch to the actual revision.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Judit Jász <jasy at inf.u-szeged.hu>'s
request for review:
Bug 20645: Duplicated constants
https://bugs.webkit.org/show_bug.cgi?id=20645

Attachment 23630: Updating the patch to the actual revision.
https://bugs.webkit.org/attachment.cgi?id=23630&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks good. Does it slow SunSpider down, or speed it up, or have no
effect?

+	 Reviewed by NOBODY (OOPS!).
+	 Duplications of constants values of CodeBlocks are eliminated.

ChangeLog should include the bug number. It's also better if the list of files
and functions below says what was done at each call site.

+    JSValue* number = NULL; 

This variable doesn't need to be initialized, because it's always set on the
next line. Also, our coding style says we use 0 instead of NULL.

Further, this can be written in a way that doesn't hash twice, using more
advanced features of the HashMap.

    pair<NumberMap::iterator, bool> addResult = m_numberMap.add(d, 0);
    JSValue* number;
    if (!addResult.second)
	number = addResult.first->second;
    else {
	number = jsNumber(globalExec(), d);
	addResult.first->second = number;
    }
    return emitLoad(dst, number);

While the above code looks a bit more confusing, it does only one hash table
lookup, while the code in your patch does two.

You can do the same thing for the string case.

+    JSValue* str = NULL; 

I'd suggest using "string" instead of "str".

+	 typedef HashMap<UString::Rep*, JSValue*, IdentifierRepHash >
IdentifierStringMap;

No need for a space before the ">" symbol.

I'm going to say r=me because my suggestions are probably optional; I think
I'll make the improvements and then test performance.