[Webkit-unassigned] [Bug 20645] Duplicated constants

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20645


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #23630|review?                     |review+
               Flag|                            |




------- Comment #8 from darin at apple.com  2008-09-21 13:26 PDT -------
(From update of attachment 23630)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.