[webkit-reviews] review denied: [Bug 40103] ThreadingPthread create thread with default stack size which might not be enough : [Attachment 57735] patch for 40103 with coding style fix.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Lyon Chen <liachen at rim.com>'s
request for review:
Bug 40103: ThreadingPthread create thread with default stack size which might
not be enough
https://bugs.webkit.org/show_bug.cgi?id=40103

Attachment 57735: patch for 40103 with coding style fix.
https://bugs.webkit.org/attachment.cgi?id=57735&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> This might not be enough.

What exactly the bug is? Secondary thread stack size is also smaller on Mac OS
X, and some may say it's "not enough".

>From all I can see, this patch just adds mysterious code that's not enabled on
any platform. Some questions the code and/or ChangeLog should help to answer
are:
- is this code still needed, or can it be removed?
- do I want this on my platform?
- if yes, how do I choose minimal stack size?

> +	   Use macro ENABLE_THREAD_STACK_SIZE_CHECK to get this code change
compiled so it will not affect most platforms. And macro
MINIMAL_THREAD_STACK_SIZE is used to define the minimal thread stack size. This
code change will only use MINIMAL_THREAD_STACK_SIZE if the default stack size
is smaller than MINIMAL_THREAD_STACK_SIZE.

This line is too long to fit in a reasonably sized window. Please stick to how
other ChangeLog entries are formatted.

+    if (stackSize < MINIMAL_THREAD_STACK_SIZE) {

It's best to include the measurement unit in constant name to avoid confusion.
Maybe MINIMAL_THREAD_STACK_BYTES?

-    if (result == 0)
+    if (!result)
	 return true;

I don't really think that changes like this are an improvement. And in any
case, it's best to not make lots of unrelated coding style changes in patches
for real bugs.

+    ASSERT(!pthread_attr_init(&attr));

This is wrong - this code won't be compiled in release builds. Same mistake is
repeated elsewhere.

+    if (!threadHandle)
+	 return 0;
+
+    return establishIdentifierForPthreadHandle(threadHandle);

No need to duplicate the common part of the function.