[webkit-reviews] review denied: [Bug 119657] JSC: Change StackIterator to not require writes to the JS stack : [Attachment 209830] patch 2: StackIterator::Frame is its own class (no longer dependent on CallFrame).

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 28 14:37:32 PDT 2013


Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 119657: JSC: Change StackIterator to not require writes to the JS stack
https://bugs.webkit.org/show_bug.cgi?id=119657

Attachment 209830: patch 2: StackIterator::Frame is its own class (no longer
dependent on CallFrame). 
https://bugs.webkit.org/attachment.cgi?id=209830&action=review

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


Looking better, but would still benefit from refinement.

> Source/JavaScriptCore/interpreter/StackIterator.cpp:66
>  void StackIterator::gotoNextFrame()

This should be called "gotoNextFrameWithFilter".

> Source/JavaScriptCore/interpreter/StackIterator.cpp:107
> +    // Hence, we're not at not an inlined frame.

Please remove the second "not".

> Source/JavaScriptCore/interpreter/StackIterator.cpp:136
> +    // Else, we're in an inlined frame.

This comment says exactly what the line of code below it says, so you should
remove it.

> Source/JavaScriptCore/interpreter/StackIterator.cpp:144
> +    m_frame.m_argumentCount = callFrame->argumentCountIncludingThis();

If you're going to store argumentCountIncludingThis, you should call it
argumentCountIncludingThis. argumentCount is a term of art for the count
excluding the implicit 'this' argument.

> Source/JavaScriptCore/interpreter/StackIterator.cpp:195
> +	   // frame would work just fine.

I don't understand what "Setting it to the physical frame would work just fine"
means. Is that what you're doing here?

> Source/JavaScriptCore/interpreter/StackIterator.cpp:204
> +void StackIterator::updateFrame()

This should be called "gotoNextFrame".

> Source/JavaScriptCore/interpreter/StackIterator.h:98
> +	   InlineCallFrame* m_inlinedFrameInfo;

It is bad when one thing becomes two.

Let's call this InlineCallFrame* "m_inlineCallFrame".

> Source/JavaScriptCore/interpreter/StackIterator.h:114
> +    inline bool operator==(Frame*);
> +    bool operator!=(Frame* frame) { return !(*this == frame); }

This is weird API. StackIterators should compare against StackIterators, not
Frame pointers.


More information about the webkit-reviews mailing list