[esnext] implement String padding proposal

[esnext] implement String padding proposal

Spec http://tc39.github.io/proposal-string-pad-start-end/

BUG=
LOG=Y
R=adamk@chromium.org, littledan@chromium.org

Committed: https://crrev.com/1a272ba23ec490f73349201c014537c851f3c964
Cr-Commit-Position: refs/heads/master@{#35061}

[caitp (gmail), 2016-02-15 23:49:51]

here's another stage 3 proposal implementation, there's no test262 coverage yet
(afaict), so the tests are probably not super adequate yet, but it's a start

[Dan Ehrenberg, 2016-02-16 15:30:35]

On 2016/02/15 at 23:49:51, caitpotter88 wrote:
> here's another stage 3 proposal implementation, there's no test262 coverage
yet (afaict), so the tests are probably not super adequate yet, but it's a start

Could you give a ping when you think this is adequate and ready for review?

[caitp (gmail), 2016-02-16 15:48:15]

On 2016/02/16 15:30:35, Dan Ehrenberg wrote:
> On 2016/02/15 at 23:49:51, caitpotter88 wrote:
> > here's another stage 3 proposal implementation, there's no test262 coverage
> yet (afaict), so the tests are probably not super adequate yet, but it's a
start
> 
> Could you give a ping when you think this is adequate and ready for review?

there's no way to know what constitutes "adequate"

[caitp (gmail), 2016-02-16 19:29:25]

On 2016/02/16 15:48:15, caitp wrote:
> On 2016/02/16 15:30:35, Dan Ehrenberg wrote:
> > On 2016/02/15 at 23:49:51, caitpotter88 wrote:
> > > here's another stage 3 proposal implementation, there's no test262
coverage
> > yet (afaict), so the tests are probably not super adequate yet, but it's a
> start
> > 
> > Could you give a ping when you think this is adequate and ready for review?
> 
> there's no way to know what constitutes "adequate"

which is another way of saying, the whole point of review is to figure out what
makes something "adequate", and comments like the initial thing are ways of
drawing attention to specific areas of a patch which may need additional
attention

[aperez, 2016-02-19 16:00:45]

On 2016/02/16 19:29:25, caitp wrote:
> On 2016/02/16 15:48:15, caitp wrote:
> > On 2016/02/16 15:30:35, Dan Ehrenberg wrote:
> > > On 2016/02/15 at 23:49:51, caitpotter88 wrote:
> > > > here's another stage 3 proposal implementation, there's no test262
> coverage
> > > yet (afaict), so the tests are probably not super adequate yet, but it's a
> > start
> > > 
> > > Could you give a ping when you think this is adequate and ready for
review?
> > 
> > there's no way to know what constitutes "adequate"
> 
> which is another way of saying, the whole point of review is to figure out
what
> makes something "adequate", and comments like the initial thing are ways of
> drawing attention to specific areas of a patch which may need additional
> attention

Popping in for an informal review...

The implementation LGTM. It would be indeed good to have test262 support, but
I think it is nicer to land this as the CL includes tests itself, instead of
letting the patch stalled here ;-)

[caitp (gmail), 2016-03-23 13:23:25]

On 2016/02/16 15:30:35, Dan Ehrenberg wrote:
> On 2016/02/15 at 23:49:51, caitpotter88 wrote:
> > here's another stage 3 proposal implementation, there's no test262 coverage
> yet (afaict), so the tests are probably not super adequate yet, but it's a
start
> 
> Could you give a ping when you think this is adequate and ready for review?

Ping

[caitp (gmail), 2016-03-23 14:06:18]

On 2016/03/23 13:23:25, caitp wrote:
> On 2016/02/16 15:30:35, Dan Ehrenberg wrote:
> > On 2016/02/15 at 23:49:51, caitpotter88 wrote:
> > > here's another stage 3 proposal implementation, there's no test262
coverage
> > yet (afaict), so the tests are probably not super adequate yet, but it's a
> start
> > 
> > Could you give a ping when you think this is adequate and ready for review?
> 
> Ping

Extra ping due to the whole #leftpad thing on twitter this morning, I guess
there's a bit of a demand for this in the stdlib now :p

[Dan Ehrenberg, 2016-03-23 15:59:49]

littledan@chromium.org changed reviewers:
+ aperez@igalia.com

[Dan Ehrenberg, 2016-03-23 15:59:50]

Sorry for the delay in the review.

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
src/js/harmony-string-padding.js:22: macro STRING_PAD_COMMON(NAME, MAXLENGTH,
FILLSTRING, S, FILLER)
Can you use a function rather than a macro here?

[caitp (gmail), 2016-03-23 17:00:41]

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
src/js/harmony-string-padding.js:22: macro STRING_PAD_COMMON(NAME, MAXLENGTH,
FILLSTRING, S, FILLER)
On 2016/03/23 15:59:50, Dan Ehrenberg wrote:
> Can you use a function rather than a macro here?

I've made a function for the "repeat the filler" segment of the macro, and the
rest is duplicated between the two functions.

I think the macro works better, but this is okay too

[Dan Ehrenberg, 2016-03-23 17:30:18]

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
src/js/harmony-string-padding.js:22: macro STRING_PAD_COMMON(NAME, MAXLENGTH,
FILLSTRING, S, FILLER)
On 2016/03/23 at 17:00:41, caitp wrote:
> On 2016/03/23 15:59:50, Dan Ehrenberg wrote:
> > Can you use a function rather than a macro here?
> 
> I've made a function for the "repeat the filler" segment of the macro, and the
rest is duplicated between the two functions.
> 
> I think the macro works better, but this is okay too

I think there could be a middle point here, which eliminates more of the
duplication but doesn't use a macro in such an unstructured way. Maybe you could
put almost all of this in a function, and simply let the caller do the
CHECK_OBJECT_COERCIBLE part? this can be taken as a parameter.

[caitp (gmail), 2016-03-23 20:45:45]

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/80001/src/js/harmony-string-p...
src/js/harmony-string-padding.js:22: macro STRING_PAD_COMMON(NAME, MAXLENGTH,
FILLSTRING, S, FILLER)
On 2016/03/23 17:30:17, Dan Ehrenberg wrote:
> On 2016/03/23 at 17:00:41, caitp wrote:
> > On 2016/03/23 15:59:50, Dan Ehrenberg wrote:
> > > Can you use a function rather than a macro here?
> > 
> > I've made a function for the "repeat the filler" segment of the macro, and
the
> rest is duplicated between the two functions.
> > 
> > I think the macro works better, but this is okay too
> 
> I think there could be a middle point here, which eliminates more of the
> duplication but doesn't use a macro in such an unstructured way. Maybe you
could
> put almost all of this in a function, and simply let the caller do the
> CHECK_OBJECT_COERCIBLE part? this can be taken as a parameter.

done

[Dan Ehrenberg, 2016-03-23 23:34:24]

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
src/js/harmony-string-padding.js:22: function StringPadCommon(self, maxLength,
fillString, fillerAtStart) {
Nit: Maybe call this StringPad or StringPadding?

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
src/js/harmony-string-padding.js:23: var S = TO_STRING(TO_OBJECT(self));
Nit: we tend to use lower-case names, like string rather than S, and we tend to
use 'receiver' or 'thisArg' rather than the Pythonism self.

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
src/js/harmony-string-padding.js:56: else return S + filler;
Sorry, one last comment: What if you just had this function return 'filler'
rather than filler + S or S + filler? You pass in a boolean and then it's only
used at the very end, so the division would work out well.

[caitp (gmail), 2016-03-23 23:36:34]

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
src/js/harmony-string-padding.js:22: function StringPadCommon(self, maxLength,
fillString, fillerAtStart) {
On 2016/03/23 23:34:24, Dan Ehrenberg wrote:
> Nit: Maybe call this StringPad or StringPadding?

Acknowledged.

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
src/js/harmony-string-padding.js:23: var S = TO_STRING(TO_OBJECT(self));
On 2016/03/23 23:34:24, Dan Ehrenberg wrote:
> Nit: we tend to use lower-case names, like string rather than S, and we tend
to
> use 'receiver' or 'thisArg' rather than the Pythonism self.

Acknowledged.

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
src/js/harmony-string-padding.js:56: else return S + filler;
On 2016/03/23 23:34:24, Dan Ehrenberg wrote:
> Sorry, one last comment: What if you just had this function return 'filler'
> rather than filler + S or S + filler? You pass in a boolean and then it's only
> used at the very end, so the division would work out well.

This presents a problem in the case on line 28, because the caller would append
the original string to the filler string

[caitp (gmail), 2016-03-23 23:37:34]

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/160001/src/js/harmony-string-...
src/js/harmony-string-padding.js:56: else return S + filler;
On 2016/03/23 23:36:34, caitp wrote:
> On 2016/03/23 23:34:24, Dan Ehrenberg wrote:
> > Sorry, one last comment: What if you just had this function return 'filler'
> > rather than filler + S or S + filler? You pass in a boolean and then it's
only
> > used at the very end, so the division would work out well.
> 
> This presents a problem in the case on line 28, because the caller would
append
> the original string to the filler string

er, it would append the original string and the original string, when it
shouldn't.

[caitp (gmail), 2016-03-24 13:57:04]

Done all of these --- it's not ideal (the macro would have been better), but at
least it's not observably wrong.

[Dan Ehrenberg, 2016-03-24 15:25:12]

This looks good--I think repeating two lines is worth it if it means we don't
have to use a macro. But I noticed a small spec compliance issue--sorry I didn't
catch it sooner.

https://codereview.chromium.org/1700003002/diff/180001/src/js/harmony-string-...
File src/js/harmony-string-padding.js (right):

https://codereview.chromium.org/1700003002/diff/180001/src/js/harmony-string-...
src/js/harmony-string-padding.js:58: var thisString =
TO_STRING(TO_OBJECT(this));
I don't see the TO_OBJECT call in the spec, just TO_STRING. This is an
observable difference.

[caitp (gmail), 2016-03-24 15:35:20]

done

[Dan Ehrenberg, 2016-03-24 15:51:04]

Description was changed from

==========
[esnext] implement String padding proposal

BUG=
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

to

==========
[esnext] implement String padding proposal

Spec http://tc39.github.io/proposal-string-pad-start-end/

BUG=
LOG=Y
R=adamk@chromium.org, littledan@chromium.org
==========

[Dan Ehrenberg, 2016-03-24 15:51:21]

lgtm

[caitp (gmail), 2016-03-24 15:54:42]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-03-24 15:54:42]

The patchset sent to the CQ was uploaded after l-g-t-m from aperez@igalia.com
Link to the patchset: https://codereview.chromium.org/1700003002/#ps200001
(title: "remove ToObject")

[commit-bot: I haz the power, 2016-03-24 15:54:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1700003002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1700003002/200001

[commit-bot: I haz the power, 2016-03-24 17:15:00]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-24 17:15:18]

Description was changed from

==========
[esnext] implement String padding proposal

Spec http://tc39.github.io/proposal-string-pad-start-end/

BUG=
LOG=Y
R=adamk@chromium.org, littledan@chromium.org
==========

to

==========
[esnext] implement String padding proposal

Spec http://tc39.github.io/proposal-string-pad-start-end/

BUG=
LOG=Y
R=adamk@chromium.org, littledan@chromium.org

Committed: https://crrev.com/1a272ba23ec490f73349201c014537c851f3c964
Cr-Commit-Position: refs/heads/master@{#35061}
==========

[commit-bot: I haz the power, 2016-03-24 17:15:19]

Patchset 11 (id:??) landed as
https://crrev.com/1a272ba23ec490f73349201c014537c851f3c964
Cr-Commit-Position: refs/heads/master@{#35061}