[webkit-reviews] review granted: [Bug 3721] Factor out AtomicString hashtable code to be usable in other cases : [Attachment 2681] Improved patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 3721: Factor out AtomicString hashtable code to be usable in other cases
http://bugzilla.opendarwin.org/show_bug.cgi?id=3721

Attachment 2681: Improved patch
http://bugzilla.opendarwin.org/attachment.cgi?id=2681&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    int size() { m_impl.count(); }

Needs a "return".

I suggest you land this with DUMP_HASHTABLE_STATS set to 0 rather than 1.

This patch still has the "_ followed by capital letter" naming scheme.

I suggest making size() and isEmpty() be const member functions.

I'm not sure I understand what the "Iterator" type is in the commented-out
code.

This patch always put the "&" next to the type name, without the space we use
before "*". You should update either the patch or the style guidelines.

I don't like the fact the "entry" variable is declared outside the loop in the
insert function. It would be better to change things so it was declared with a
value, rather than declared and then later initialized.

Some single-line if statement bodies have braces, for example:

+	 if (equal(entry, key)) {
+	     return entry;
+	 }

Perhaps you should add a "consistency check" mode like the one I have in
KJS::PropertyMap. That has been quite valuable to me in debugging the hash
table implementation, and vetting changes.

The template doesn't document that the key needs to be a POD type.

All those interfaces that take functions that in turn take const & to their
values create a penalty for simple types like pointers an integers. If we could
find some way to make hash functions take plain old "DOMStringImpl *" rather
than "DOMStringImpl * const&" we would get slightly better performance for
these simple cases. Not that I have an idea how to do that.

But r=me, assuming you fix a few of the small things still outstanding.