[webkit-reviews] review denied: [Bug 87334] Dynamic hash table in DOMObjectHashTableMap is wrong in multiple threads : [Attachment 145927] Patch v3

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Leo Yang
<leo.yang at torchmobile.com.cn>'s request for review:
Bug 87334: Dynamic hash table in DOMObjectHashTableMap is wrong in multiple
threads
https://bugs.webkit.org/show_bug.cgi?id=87334

Attachment 145927: Patch v3
https://bugs.webkit.org/attachment.cgi?id=145927&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=145927&action=review


> It seems that Mac build doesn't allow global initializer for most files.

Indeed, it doesn't. I'm surprised that constant initialization requires a
global initializer. I guess we have to go back to struct initialization. Let's
use a "copy" function in JSC::HashTable instead of a copy constructor.

> Source/JavaScriptCore/runtime/Lookup.h:122
> +	   { }

WebKit style is to put a newline between "{" and "}".

> Source/JavaScriptCore/runtime/Lookup.h:127
> +	       if (this == &other)
> +		   return;

Since this is not the assignment operator, this == &other should be impossible.


> Source/JavaScriptCore/runtime/Lookup.h:132
> +	       compactSize = other.compactSize;
> +	       compactHashSizeMask = other.compactHashSizeMask;
> +	       values = other.values;
> +	       // Don't copy dynamic table since it's thread specific.
> +	       table = 0;

Please use initializer syntax here.