[webkit-reviews] review denied: [Bug 117390] Introducing StackIterator class. : [Attachment 204743] revised patch as a delta from rolled out patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 117390: Introducing StackIterator class.
https://bugs.webkit.org/show_bug.cgi?id=117390

Attachment 204743: revised patch as a delta from rolled out patch
https://bugs.webkit.org/attachment.cgi?id=204743&action=review

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


exec->find(exec) is a strange design pattern. We're not looking for exec -- we
already have it. exec->begin() would be better.

> Source/JavaScriptCore/interpreter/CallFrame.cpp:102
>      if (!this)

When can 'this' be NULL? This is a strange design pattern. Usually, invoking a
member function on a null pointer is not allowed.

> Source/JavaScriptCore/interpreter/CallFrame.cpp:109
>      if (!this)

Ditto.

> Source/JavaScriptCore/interpreter/CallFrame.cpp:116
> +    if (!this)

Ditto.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:413
> +    StackIterator iter = callFrame->find(callFrame);
> +    ++iter;

Don't we need to check for end() here?

> Source/JavaScriptCore/interpreter/StackIterator.cpp:48
> +    if (m_vm)

When can vm be NULL?

> Source/JavaScriptCore/interpreter/StackIterator.cpp:57
> +    if (m_vm)

Ditto.

> Source/JavaScriptCore/interpreter/StackIterator.cpp:66
> +    if (m_vm) {

Ditto.

> Source/JavaScriptCore/interpreter/StackIterator.cpp:76
> +    if (!frame)

When can frame be NULL?

> Source/JavaScriptCore/runtime/InitializeThreading.cpp:71
> +    StackIterator::initializeStatics();

This is no longer needed.

> Source/JavaScriptCore/runtime/JSFunction.cpp:208
>      ++iter;

Don't we need to check for end() here?

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:716
>      ++iter;

Ditto.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:734
>      ++iter;

Ditto.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:144
>      ++iter;

Ditto.