[webkit-reviews] review denied: [Bug 91899] [BlackBerry] Merge createThreadInternal implementations : [Attachment 153601] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 20 14:58:42 PDT 2012
Alexey Proskuryakov <ap at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 91899: [BlackBerry] Merge createThreadInternal implementations
https://bugs.webkit.org/show_bug.cgi?id=91899
Attachment 153601: Patch
https://bugs.webkit.org/attachment.cgi?id=153601&action=review
------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=153601&action=review
> Source/WTF/wtf/ThreadingPthreads.cpp:171
> +static size_t threadStackSize()
I suggest naming this something like desiredSecondaryThreadStackSize. Comments
from bug 40103 still apply - if you are adding this to cross platform code, you
need to explain how to choose the desired size.
> Source/WTF/wtf/ThreadingPthreads.cpp:195
> + else if (pthread_attr_setstack(&attr, stackAddr, stackSize))
> LOG_ERROR("pthread_attr_getstack() failed: %d", errno);
The error message mentions a wrong function.
Why is error handling different here? If pthread_attr_init fails, the thread is
not created, but here, errors are ignored.
What guarantees that there is enough space for a larger stack at stackAddr?
> Source/WTF/wtf/ThreadingPthreads.cpp:204
> pthread_setname_np(threadHandle, threadName);
This will break build for platforms that don't have pthread_np.h, or don't have
this particular function. Besides, I'm fairly sure that other ports give names
to threads elsewhere.
More information about the webkit-reviews
mailing list