[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