[webkit-reviews] review granted: [Bug 37978] Unify JSC::IdentifierTable and WebCore::AtomicStringTable implementations. : [Attachment 54038] Fix review bot issues

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 37978: Unify JSC::IdentifierTable and WebCore::AtomicStringTable
implementations.
https://bugs.webkit.org/show_bug.cgi?id=37978

Attachment 54038: Fix review bot issues
https://bugs.webkit.org/attachment.cgi?id=54038&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
Looks good. 

A few hashtable comments:


+    LiteralTable* literalTable = globalData->literalTable;
+    const LiteralTable::iterator& iter = literalTable->find(c);
+    if (iter != literalTable->end())
	 return iter->second;
 
-    pair<HashSet<UString::Rep*>::iterator, bool> addResult =
identifierTable.add<const char*, IdentifierCStringTranslator>(c);
-
-    // If the string is newly-translated, then we need to adopt it.
-    // The boolean in the pair tells us if that is so.
-    RefPtr<UString::Rep> addedString = addResult.second ?
adoptRef(*addResult.first) : *addResult.first;
-
-    literalIdentifierTable.add(c, addedString.get());
-
+    RefPtr<StringImpl> addedString = globalData->identifierTable->add(c);
+    literalTable->add(c, addedString.get());
     return addedString.release();

I'm surprised that we have a separate table for char*. Weird. Maybe it's just
legacy code that can go away?

Anyhoo, "literalTable->find" followed by "literalTable->add" is a
double-lookup. The idiom for avoiding double-lookup is:

pair<iterator, bool> result = literalTable->add(c, 0);
if (!result.second) // pre-existing entry
    return result.first->second;
...
result.first->second = addedString.get();


+template<bool isIdentifierTable>
+IdentifierOrAtomicStringTable<isIdentifierTable>::~IdentifierOrAtomicStringTab
le()
+{
+    HashSet<StringImpl*>::iterator end = m_table.end();
+    for (HashSet<StringImpl*>::iterator iter = m_table.begin(); iter != end;
++iter) {
+	 if (isIdentifierTable)
+	     (*iter)->setIsIdentifier(false);
+	 else
+	     (*iter)->setIsAtomic(false);
+    }
+}

This would probably be more efficient if you wrote two copies of the loop, and
moved the isIdentifierTable test out of the loop. We know that
isIdentifierTable is constant inside the loop, but GCC probably doesn't.

r=me