[webkit-reviews] review requested: [Bug 120564] Converting StackIterator to a callback interface : [Attachment 210393] patch 3: removed StackIterator::find().

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has asked  for review:
Bug 120564: Converting StackIterator to a callback interface
https://bugs.webkit.org/show_bug.cgi?id=120564

Attachment 210393: patch 3: removed StackIterator::find().
https://bugs.webkit.org/attachment.cgi?id=210393&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>


(In reply to comment #11)
> (From update of attachment 210232 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=210232&action=review
> 
> I'm worried about StackIterator::find() - does this method need to exist?

StackIterator::find() is now removed in patch 3.

> > Source/JavaScriptCore/interpreter/StackIterator.h:119
> > +	 class Functor {
> > +	 public:
> > +	     Status operator()(StackIterator&) { return Done; }
> > +	 };
> 
> I don't think you need to declare this.  And you don't need the various
implementations to subclass this.

The only reason I wanted to provide this is because it makes it clear to the
implementers of Functor subclasses as to what the prototype the functor is
expected to have.  Sure, it can be inferred from the implementation of
StackIterator::iterate() (or from other subclasses of StackIterator::Functor),
but I think it's better to state it explicitly as the expected interface.  I
change the code to just be a prototype without an implementation, and added a
comment to indicate that the subclass should implement that method.