Bug 120695 – Refining the StackIterator callback interface

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Addressing Geoff's feedback in https://bugs.webkit.org/show_bug.cgi?id=120564#c17.

[Mark Lam]

Created attachment 210487 [details]
the patch.

This patch has run the javascriptcore and layout tests with no regressions.

[Geoffrey Garen]

Comment on attachment 210487 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=210487&action=review

> Source/JavaScriptCore/interpreter/CallFrame.h:292
> +            StackIterator iter(this);
> +            iter.iterate<Functor>(functor);

Why do we still need to instantiate this object here?

[EFL EWS Bot]

Comment on attachment 210487 [details]
the patch.

Attachment 210487 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1693729

[Geoffrey Garen]

Comment on attachment 210487 [details]
the patch.

After an in-person conversation:

This patch is an improvement, so I'm going to mark it r+.

Please follow up with a rename from StackIterator to StackVisitor, to avoid confusion with the C++ term "iterator".

I don't think we've achieved the callback style of implementation that we originally discussed, but I'm not sure it's super important to get there at this point.

[Mark Lam]

(In reply to comment #3)
> (From update of attachment 210487 [details])
> Attachment 210487 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/1693729

The EFL WK2 build failure does not look like it's related to this patch.

[Mark Lam]

Thanks for the review.  Landed in r155075: <http://trac.webkit.org/r155075>.

Follow up patch to rename StackIterator to StackVisitor to come shortly.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 210487 [details])
> After an in-person conversation:
> 
> This patch is an improvement, so I'm going to mark it r+.
> 
> Please follow up with a rename from StackIterator to StackVisitor, to avoid confusion with the C++ term "iterator".
> 
> I don't think we've achieved the callback style of implementation that we originally discussed, but I'm not sure it's super important to get there at this point.

I like what we have now.  It achieves the goal of allowing the iterator implementation to change more flexibly without having to make changes to its clients.