[webkit-reviews] review denied: [Bug 40089] Add a takeFirst() method to Deque and use it where appropriate : [Attachment 57716] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tony Gentilcore <tonyg at chromium.org>'s
request for review:
Bug 40089: Add a takeFirst() method to Deque and use it where appropriate
https://bugs.webkit.org/show_bug.cgi?id=40089

Attachment 57716: Patch
https://bugs.webkit.org/attachment.cgi?id=57716&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    template<typename T>
> +    inline T& Deque<T>::takeFirst()
> +    {
> +	   T& temp = first();
> +	   removeFirst();
> +	   return temp;
> +    }

This won't work. This returns a reference to the place in memory where the
first item was stored, but once removeFirst is called that location in memory
no longer contains an object. The return type of takeFirst needs to be T, not
T&, and the local variable, which should not be named "temp" since we don't use
abbreviations like that in WebKit, should also be of type T. I would write it
like this:

    template<typename T>
    inline T Deque<T>::takeFirst()
    {
	T oldFirst = first();
	removeFirst();
	return oldFirst;
    }

The only reason this seemed to be working is that we got lucky. The storage the
object had been in was hanging around just long enough to not immediately
crash.