[webkit-reviews] review denied: [Bug 3721] Factor out AtomicString hashtable code to be usable in other cases : [Attachment 2650] Initial patch for HashSet template

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied 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 2650: Initial patch for HashSet template
http://bugzilla.opendarwin.org/attachment.cgi?id=2650&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Too much of these template classes is declared inside the class definitions
themselves. Doing this makes the functions all behave as if they were marked
"inline", which is not desirable for functions like HashSetImp::insert.

I'd like to see values like the minimum table size, the expand and shrink
factors, and the expand and shrink thresholds be parameters somehow, although
it would probaly require some kind of traits approach to prevent the class from
bloat.

Identifiers with underscores followed by capital letters are reserved for the
C++ library implementation, so we should probably avoid those.

The way HashSet is implemented as a shared pointer to an implementation object
seems a little strange to me. This prevents copying the hash table, which seems
unnecessary -- why the extra overhead of dereferencing a pointer all the time?
Since the entire implementation of HashSet is in the header, I don't see the
value in splitting it into a separate smart pointer class backed by an impl.

I'd like to see the hash and equal functions for DOMStringImpl in the
DOMStringImpl header, since they are presumably useful for more than just the
AtomicString hash table. Or perhaps compatible equality and hash functions for
QString, DOMString, DOMStringImpl, char *, etc. could all go in a single
header.

The code uses the "* or & next to the class name" format for some of this new
code. While I'm fond of this style, it's not what is mentioned in the coding
style document.

I think the inlining issue is important enough that it should be fixed before
landing, so I'm setting review-.