[Webkit-unassigned] [Bug 20645] Duplicated constants

[bugzilla-daemon at webkit.org]

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





------- Comment #4 from ggaren at apple.com  2008-09-18 08:58 PDT -------
Thanks, Judit, this latest patch looks a lot better.

Three comments:

1. I'd like to improve the string map a bit. Right now, the typedef is 

+        typedef HashMap<RefPtr<UString::Rep>, JSValue*> StringMap;

We can get rid of the RefPtr in that typedef. The string is owned by the syntax
tree, so there's no need for the code generator to ref and deref it.

Also, because we know that we're hashing Identifier strings, we can use
IdentifierRepHash, which is optimized for the case where the string has already
computed a hash code.

2. Though your patch didn't introduce this problem, it made me realize that
emitLoad for UString and its use are a little weird:

+        RegisterID* emitLoad(RegisterID* dst, const UString&);

-    return generator.emitLoad(dst, jsOwnedString(generator.globalExec(),
Identifier(generator.globalExec(), m_value).ustring()));
+    return generator.emitLoad(dst, m_value);

emitLoad is declared to operate on UStrings, but in fact it only wants to
operate on Identifiers!

A more efficient solution would be to declare emitLoad to operate on an
Identifier: "RegisterID* emitLoad(RegisterID* dst, const Identifier&);". And
StringNode, instead of holding m_value as a UString, should hold m_value as an
Identifier. That way, we won't have to make a temporary Identifier every time
we encounter a string in the syntax tree.

3. A simpler way to write

+    NumberMap::iterator iterator = m_numberMap.find(d);
+    if (iterator != m_numberMap.end())
+        return emitLoad(dst, iterator->second);

is

if (JSValue* v = m_numberMap.get(d))
    return emitLoad(dst, v);

The same goes for emitLoad of a string.

I'm going to say r- because I think these would be good improvements to make,
but overall this patch looks really good!


-- 
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.