Implemented String.prototype.repeat as per ES6 draft 07-15-13, section 15.5.3.21

Implemented String.prototype.repeat as per ES6 draft 07-15-13, section 15.5.3.21

TEST=mjsunit/string-repeat
BUG=v8:2799

[Michael Starzinger, 2013-07-29 12:44:53]

Sven has graciously agreed to take this one over. I'll take a look at the
"endsWith" and "startsWith" ones. Thanks.

[Sven Panne, 2013-07-29 13:27:53]

NOT LGTM. General advice: Follow the spec closely step by step, almost
everything is observable in JavaScript, so there is not much room for variation
and clever optimizations. Write unit tests to check correct sequencing of
operations.

Furthermore, this should be behind the --harmony flag.

https://codereview.chromium.org/20818005/diff/1/src/string.js
File src/string.js (right):

https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode891
src/string.js:891: 
We have to call TO_STRING_INLINE here, not later, because this is observable.
Please add a test case for this, too.

https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode892
src/string.js:892: if (IS_NULL_OR_UNDEFINED(count) && !IS_UNDETECTABLE(count)) {
Why test this? What should really happen at that point is ToInteger(count),
which is observable. Again: Test case!

https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode898
src/string.js:898: if (s.length === 0) {
This optimization is not allowed by the spec, at least not if something
observable comes after it in the final implementation.

https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode908
src/string.js:908: if (n === 0) {
I am not sure if a zero count is a common case, we should probably remove that.

https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode913
src/string.js:913: var i = 0;
Style nit: Use a for loop for readability.

[ioseb, 2013-07-29 23:24:00]

Thanks for review Sven!

As was suggested implementation was moved to separate file and i reuploaded
patch: https://codereview.chromium.org/21014007

I've made all suggested changes there.


On 2013/07/29 13:27:53, Sven Panne wrote:
> NOT LGTM. General advice: Follow the spec closely step by step, almost
> everything is observable in JavaScript, so there is not much room for
variation
> and clever optimizations. Write unit tests to check correct sequencing of
> operations.
> 
> Furthermore, this should be behind the --harmony flag.
> 
> https://codereview.chromium.org/20818005/diff/1/src/string.js
> File src/string.js (right):
> 
> https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode891
> src/string.js:891: 
> We have to call TO_STRING_INLINE here, not later, because this is observable.
> Please add a test case for this, too.
> 
> https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode892
> src/string.js:892: if (IS_NULL_OR_UNDEFINED(count) && !IS_UNDETECTABLE(count))
{
> Why test this? What should really happen at that point is ToInteger(count),
> which is observable. Again: Test case!
> 
> https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode898
> src/string.js:898: if (s.length === 0) {
> This optimization is not allowed by the spec, at least not if something
> observable comes after it in the final implementation.
> 
> https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode908
> src/string.js:908: if (n === 0) {
> I am not sure if a zero count is a common case, we should probably remove
that.
> 
> https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode913
> src/string.js:913: var i = 0;
> Style nit: Use a for loop for readability.

[Sven Panne, 2013-07-30 06:41:32]

Superseded by https://codereview.chromium.org/21014007, closing.