[webkit-reviews] review denied: [Bug 45060] Add unit tests for red-black tree and (POD) arena : [Attachment 66464] Revised patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 45060: Add unit tests for red-black tree and (POD) arena
https://bugs.webkit.org/show_bug.cgi?id=45060

Attachment 66464: Revised patch
https://bugs.webkit.org/attachment.cgi?id=66464&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66464&action=prettypatch

> WebKit/chromium/tests/PODArenaTest.cpp:32
> +// Tests for the PODArena.
nit: I'd nuke this comment since it is redundant with the file name.

> WebKit/chromium/tests/PODArenaTest.cpp:82
> +protected:
nit: looks like this constructor can just be in the private section

> WebKit/chromium/tests/PODArenaTest.cpp:115
> +    for (int i = 0; i < 10000; ++i)
this choice of 10000 seems to depend on the value of DefaultChunkSize.
maybe you should parameterize this using that value?  this test could
fail if DefaultChunkSize were increased significantly.

> WebKit/chromium/tests/PODRedBlackTreeTest.cpp:44
> +using namespace TreeTestHelpers;
instead of a namespace, I think a class with static methods would be 
better.  it is just uncommon in webkit to use a namespace like this.

> WebKit/chromium/tests/PODRedBlackTreeTest.cpp:147
> +    tree.add(5113);
is there something special about the choice of numbers here?
maybe a comment would be good if so?  elsewhere, you use
smaller numbers :)

> WebKit/chromium/tests/PODRedBlackTreeTest.cpp:168
> +	   int val = nextRandom(maximumValue);
nit: spell out "value"

> WebKit/chromium/tests/PODRedBlackTreeTest.cpp:176
> +	   int idx = nextRandom(treeSize);
nit: spell out "index" and "value"