Allow multi-codeunit padding in String.padLeft/padRight.

Allow multi-codeunit padding in String.padLeft/padRight.

Also allow negative repeats.

Based on suggestions from Stephen.

R=sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32843

[Lasse Reichstein Nielsen, 2014-02-19 15:01:37]

This makes s.padLeft(n, p) really equal to p.repeat(n - s.length) + s 

And it works. I agree that the {size} parameter was probably not worth it.
(This also cleans up the existing code, because there is a couple of leftover
{size} parameters).

[floitsch, 2014-02-19 15:11:10]

I still think that string.pad should end up with the size the user requests, but
I will not veto Stephen's ok.

[Lasse Reichstein Nielsen, 2014-02-19 15:21:29]

This makes s.padLeft(n, p) really equal to p.repeat(n - s.length) + s 

And it works. I agree that the {size} parameter was probably not worth it.
(This also cleans up the existing code, because there is a couple of leftover
{size} parameters).

[Lasse Reichstein Nielsen, 2014-02-19 15:22:12]

As a suggestion: Make repeat be operator*.
This turns it into proper scalar multiplication, the logical equivalent of
repeated addition.

It reads quite well to write 
  "xyz" * 5
instead of 
  "xyz".repeat(5)
It (and String.+) should be made compile time constants.

[floitsch, 2014-02-19 15:25:02]

On 2014/02/19 15:22:12, Lasse Reichstein Nielsen wrote:
> As a suggestion: Make repeat be operator*.
> This turns it into proper scalar multiplication, the logical equivalent of
> repeated addition.
> 
> It reads quite well to write 
>   "xyz" * 5
> instead of 
>   "xyz".repeat(5)
> It (and String.+) should be made compile time constants.

Personally I'm not against it, but I'm afraid of unintended consequences (for
example in dart2js).

[sra1, 2014-02-19 18:09:02]

On 2014/02/19 15:25:02, floitsch wrote:
> On 2014/02/19 15:22:12, Lasse Reichstein Nielsen wrote:
> > As a suggestion: Make repeat be operator*.
> > This turns it into proper scalar multiplication, the logical equivalent of
> > repeated addition.
> > 
> > It reads quite well to write 
> >   "xyz" * 5
> > instead of 
> >   "xyz".repeat(5)
> > It (and String.+) should be made compile time constants.
> 
> Personally I'm not against it, but I'm afraid of unintended consequences (for
> example in dart2js).

I though about potential problems with operator*
Duration and Point already have operator*
In box2d, all the vectors and matrixes have operator*
So I think we have already lost the ability to see 'c = a * b' with dynamic a
and infer anything useful about the type of c.

I could find no code in swarm or mixer where we could have strengthened any
types surrounding a multiply.
It does not mean we are free of consequences, but after looking, I am less
worried that this single step has consequences.

[sra1, 2014-02-19 21:18:09]

lgtm

I'd like to see the magic .join code go away but that can be a follow-up.

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_string.dart (right):

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_string.dart:232: return JS('String', "new Array(# +
1).join(#)", times, this);
I'm a bit nervous of this idiom (in all three places).

AFAIK from looking at the V8 code, join is only optimized for lists of ascii
strings, which this is not.  I think you end up with another temporary
allocation here to hold the stringified elements:
https://code.google.com/p/v8/codesearch#v8/trunk/src/array.js&l=147

Some kind of length-doubling algorithm is likely to work well.

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_string.dart:239: list[delta] = this;
Current range analysis does not seem to eliminate this bounds check.

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_string.dart:240: return JS("String", "#.join(#)", list,
padding);
Single quotes to match the rest of the file.

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/core/string.dart...
sdk/lib/core/string.dart:346: * no padding will happen. A negative `newLength`
is allowed,
Maybe: happen -> be added.

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/core/string.dart...
sdk/lib/core/string.dart:356: String padLeft(int newLength, [String padding = '
']);
I think 'width' is a better name than 'newLength'.
We pad something 'to a width', and by using different roots, 'width' and
'length', it feels more comfortable that the input and output may be different
conceptual units.

E.g. it sounds better to say

  "the result will not have length `width`"

than

  "the result will not have length `newLength`"

[sra1, 2014-02-19 23:15:11]

I reworked the JavaScript implementations for a 5x-8x speed increase.

Can you double-check and land this?
Only the JavaScript file is changed.
I didn't change any variable names or documentation.

https://codereview.chromium.org/172543003/

https://codereview.chromium.org/171773003/diff/50001/tests/corelib/string_tes...
File tests/corelib/string_test.dart (right):

https://codereview.chromium.org/171773003/diff/50001/tests/corelib/string_tes...
tests/corelib/string_test.dart:413: 3,
I would add some more, at least covering any special kind of string building.
All numbers in range 0..16?

[Lasse Reichstein Nielsen, 2014-02-20 07:16:40]

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_string.dart (right):

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_string.dart:232: return JS('String', "new Array(# +
1).join(#)", times, this);
I've incorporated your CL, so all this goes away.

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/core/string.dart...
sdk/lib/core/string.dart:346: * no padding will happen. A negative `newLength`
is allowed,
On 2014/02/19 21:18:09, sra1 wrote:
> Maybe: happen -> be added.

Done.

https://codereview.chromium.org/171773003/diff/50001/sdk/lib/core/string.dart...
sdk/lib/core/string.dart:356: String padLeft(int newLength, [String padding = '
']);
On 2014/02/19 21:18:09, sra1 wrote:
> I think 'width' is a better name than 'newLength'.
> We pad something 'to a width', and by using different roots, 'width' and
> 'length', it feels more comfortable that the input and output may be different
> conceptual units.
> 
> E.g. it sounds better to say
> 
>   "the result will not have length `width`"
> 
> than
> 
>   "the result will not have length `newLength`"
> 
> 

Done.

https://codereview.chromium.org/171773003/diff/50001/tests/corelib/string_tes...
File tests/corelib/string_test.dart (right):

https://codereview.chromium.org/171773003/diff/50001/tests/corelib/string_tes...
tests/corelib/string_test.dart:413: 3,
Added all numbers in range 0..17 and replaced 100 by 127,128,129.

https://codereview.chromium.org/171773003/diff/170001/runtime/lib/string_patc...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/171773003/diff/170001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:666: String operator*(int times) {
These one-byte optimizations are missing from _ExternalOneByteString.
Would it make sense to have a _OneByteStringBase that can be inherited by all
one byte strings?

[Lasse Reichstein Nielsen, 2014-02-20 07:18:06]

Committed patchset #4 manually as r32843 (presubmit successful).

[Lasse Reichstein Nielsen, 2014-02-20 13:29:36]

+iposva since srdjan is otherwise engaged.