[webkit-reviews] review denied: [Bug 65399] StackBounds checker fails on custom stack implementations (typically in a coroutine setting) : [Attachment 102423] Proposed patch v2 (fixed style errors in Changelog)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jul 30 10:44:44 PDT 2011


Darin Adler <darin at apple.com> has denied Slava Akhmechet
<coffeemug at gmail.com>'s request for review:
Bug 65399: StackBounds checker fails on custom stack implementations (typically
in a coroutine setting)
https://bugs.webkit.org/show_bug.cgi?id=65399

Attachment 102423: Proposed patch v2 (fixed style errors in Changelog)
https://bugs.webkit.org/attachment.cgi?id=102423&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102423&action=review


I think that Geoff Garen or Oliver Hunt should take a look at this before we
land it. Geoff will be away for a while because he is getting married today and
is going on a honeymoon, so Oliver may be the best bet.

Mark Rowe may also be a good person to help us properly add new API getting the
version checking right and having it exported properly.

> Source/JavaScriptCore/API/JSContextRef.cpp:70
> +    RefPtr<JSGlobalData> globalData = PassRefPtr<JSGlobalData>(toJS(group));

> +    globalData->stack()->setBounds(origin, bound);

There is no reason to put this into a local variable or use a RefPtr. It’s also
not helpful and wrong style to use PassRefPtr here at all. It should just be
toJS(group)->stack()->setBounds

> Source/JavaScriptCore/API/JSContextRef.h:80
> +JS_EXPORT void JSSetStackBounds(JSContextGroupRef, void *origin, void
*bound) AVAILABLE_IN_WEBKIT_VERSION_4_0;

AVAILABLE_IN_WEBKIT_VERSION_4_0 is incorrect. This is something we just added,
so it wasn’t available before.

In this file arguments need names, because it’s a C header file, not C++, so
JSContextGroupRef needs the name "group" as you see above in other functions.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:588
> -	   StackBounds m_stack;
> +	   StackBounds *m_stack;

Formatting here is incorrect. The * goes next to the type.

> Source/JavaScriptCore/runtime/JSGlobalData.h:294
> -	   StackBounds m_stack;
> +	   StackBounds *m_stack;

Same problem with placement of the *.

> Source/JavaScriptCore/wtf/StackBounds.h:81
> +    void setBounds(void *origin, void *bound)

Incorrect formatting here. It's void* not void * in WebKit code.


More information about the webkit-reviews mailing list