Add StringSink and update StringBuffer.

Add StringSink and update StringBuffer.

Committed: https://code.google.com/p/dart/source/detail?r=18313
Reverted: http://code.google.com/p/dart/source/detail?r=18327

Committed: https://code.google.com/p/dart/source/detail?r=18626
Reverted: http://code.google.com/p/dart/source/detail?r=18629

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

[floitsch, 2013-02-09 00:59:02]

Tests and users still need to get updated.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.dart
File sdk/lib/core/string_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:64: @deprecated
Not sure about this.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:74: @deprecated
Not sure about this.

[kasperl, 2013-02-11 07:51:53]

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_sink.dart
File sdk/lib/core/string_sink.dart (right):

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_sink.dart...
sdk/lib/core/string_sink.dart:26: void addCharCode(int charCode);
Wouldn't writeCharCode be more consistent?

[Lasse Reichstein Nielsen, 2013-02-11 12:30:11]

lgtm

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.dart
File sdk/lib/core/string_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:15: StringBuffer([Object content = ""]) {
Just make the constructor external.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:51: write("\n");
_write for the second call.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:56: write(new String.fromCharCode(charCode));
_write

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:62: * *Deprecated*. Use
`objects.forEach(buffer.write)` instead.
Backquotes are not part of the dartdoc spec. Use [::] instead.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:64: @deprecated
Agree, I'd vote for removing it.
Let the iterable do the iteration, not us.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:74: @deprecated
I'm not sure we should have a clear method at all.
It's better to just create a new sink if you need it.
Having a destructive operation like this on an object you might want to pass to
other code isn't safe.

If we have it, how about giving it an optional position argument, so you can
reset to any previous length.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_sink.dart
File sdk/lib/core/string_sink.dart (right):

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_sink.dart...
sdk/lib/core/string_sink.dart:26: void addCharCode(int charCode);
Agree.

[floitsch, 2013-02-11 17:10:49]

Needed to update the code-buffer class.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.dart
File sdk/lib/core/string_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:15: StringBuffer([Object content = ""]) {
On 2013/02/11 12:30:11, Lasse Reichstein Nielsen wrote:
> Just make the constructor external.

Done.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:51: write("\n");
On 2013/02/11 12:30:11, Lasse Reichstein Nielsen wrote:
> _write for the second call.

Done.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:56: write(new String.fromCharCode(charCode));
On 2013/02/11 12:30:11, Lasse Reichstein Nielsen wrote:
> _write

Done.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:62: * *Deprecated*. Use
`objects.forEach(buffer.write)` instead.
On 2013/02/11 12:30:11, Lasse Reichstein Nielsen wrote:
> Backquotes are not part of the dartdoc spec. Use [::] instead.

Done.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_buffer.da...
sdk/lib/core/string_buffer.dart:74: @deprecated
On 2013/02/11 12:30:11, Lasse Reichstein Nielsen wrote:
> I'm not sure we should have a clear method at all.
> It's better to just create a new sink if you need it.
> Having a destructive operation like this on an object you might want to pass
to
> other code isn't safe.
> 
> If we have it, how about giving it an optional position argument, so you can
> reset to any previous length.

Let's deprecate it then.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_sink.dart
File sdk/lib/core/string_sink.dart (right):

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_sink.dart...
sdk/lib/core/string_sink.dart:26: void addCharCode(int charCode);
On 2013/02/11 07:51:53, kasperl wrote:
> Wouldn't writeCharCode be more consistent?

Done.

https://codereview.chromium.org/12218086/diff/1/sdk/lib/core/string_sink.dart...
sdk/lib/core/string_sink.dart:26: void addCharCode(int charCode);
On 2013/02/11 12:30:11, Lasse Reichstein Nielsen wrote:
> Agree.

Done.

[floitsch, 2013-02-13 14:15:05]

PTAL.
added Peter (for dart2js)
added Srdjan (for the VM. sorry I forgot you the first time).

https://codereview.chromium.org/12218086/diff/11003/runtime/lib/string_buffer...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/12218086/diff/11003/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:11: _buffer = new List<String>();
Inlined the call to clear, since 'clear' is deprecated and will be removed
eventually.

[srdjan, 2013-02-13 18:22:39]

LGTM for runtime changes and thanks for adding me.

[floitsch, 2013-02-15 10:40:27]

@lrn: PTAL
@peter: ping.

[ahe, 2013-02-15 11:30:41]

There are a lot of changes in this CL that makes me nervous. Did you check that
you didn't regress on the benchmark?

https://chromiumcodereview.appspot.com/12218086/diff/11003/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://chromiumcodereview.appspot.com/12218086/diff/11003/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:261: _contents =
Primitives.stringConcat(_contents, str);
I think this should be called stringConcatUnchecked.

[Lasse Reichstein Nielsen, 2013-02-15 16:12:27]

Still LGTM

[floitsch, 2013-02-18 14:15:40]

Needed to add writeAll to CodeBuffer.
Resubmitting.

@ahe: I looked at the generated JS code and the call to stringConcatUnchecked is
inlined. From what I can see this would be the main-reason for slow-downs. So
I'm optimistically submitting. If we see regressions we should be able to fix
them.

https://codereview.chromium.org/12218086/diff/11003/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/12218086/diff/11003/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:261: _contents =
Primitives.stringConcat(_contents, str);
On 2013/02/15 11:30:41, ahe wrote:
> I think this should be called stringConcatUnchecked.

Done.

[Ivan Posva, 2013-02-18 15:39:48]

Please make sure that the benchmarks are updated to avoid breakage when the
deprecated methods are removed.

-Ivan

On 2013/02/18 14:15:40, floitsch wrote:
> Needed to add writeAll to CodeBuffer.
> Resubmitting.
> 
> @ahe: I looked at the generated JS code and the call to stringConcatUnchecked
is
> inlined. From what I can see this would be the main-reason for slow-downs. So
> I'm optimistically submitting. If we see regressions we should be able to fix
> them.
> 
>
https://codereview.chromium.org/12218086/diff/11003/sdk/lib/_internal/compile...
> File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):
> 
>
https://codereview.chromium.org/12218086/diff/11003/sdk/lib/_internal/compile...
> sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:261: _contents =
> Primitives.stringConcat(_contents, str);
> On 2013/02/15 11:30:41, ahe wrote:
> > I think this should be called stringConcatUnchecked.
> 
> Done.

[sra1, 2013-02-18 21:06:57]

DBC

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/code_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/code_buffer.dart:7: class CodeBuffer
implements StringBuffer {
It would be better if the methods that implement methods from StringBuffer had
compatible signatures.
The methods here return CodeBuffer, but the ones from StringBuffer / StringSink
return void.

You might have to change the call sites to use cascades.

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/code_buffer.dart:48: return
write(object).write("\n");
If you really need to chain here, use cascades.

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/core/string_buffe...
File sdk/lib/core/string_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/core/string_buffe...
sdk/lib/core/string_buffer.dart:17: /// Returns the length of the buffer.
Does it return the length of the buffer (could have additional capacity), or the
length of the string thus far accumulated?

Are you guaranteeing this is constant time? (The use in isEmpty would hint that
is so, but please state.)

[ahe, 2013-02-19 12:11:43]

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/core/string_buffe...
File sdk/lib/core/string_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/core/string_buffe...
sdk/lib/core/string_buffer.dart:31: external void write(Object obj);
You forgot to document all the new methods you added.

[floitsch, 2013-02-19 14:21:17]

Changes in:
https://codereview.chromium.org/12303026

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/code_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/code_buffer.dart:7: class CodeBuffer
implements StringBuffer {
On 2013/02/18 21:06:57, sra1 wrote:
> It would be better if the methods that implement methods from StringBuffer had
> compatible signatures.
> The methods here return CodeBuffer, but the ones from StringBuffer /
StringSink
> return void.
> 
> You might have to change the call sites to use cascades.

Agreed. The CodeBuffer already diverged, so I just followed its conventions, but
it should be modified.

Filed issue 8607.

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/code_buffer.dart:48: return
write(object).write("\n");
On 2013/02/18 21:06:57, sra1 wrote:
> If you really need to chain here, use cascades.

Will do, when I refactor the CodeBuffer.

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/core/string_buffe...
File sdk/lib/core/string_buffer.dart (right):

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/core/string_buffe...
sdk/lib/core/string_buffer.dart:17: /// Returns the length of the buffer.
On 2013/02/18 21:06:57, sra1 wrote:
> Does it return the length of the buffer (could have additional capacity), or
the
> length of the string thus far accumulated?
> 
> Are you guaranteeing this is constant time? (The use in isEmpty would hint
that
> is so, but please state.)
> 

Done.

https://codereview.chromium.org/12218086/diff/23001/sdk/lib/core/string_buffe...
sdk/lib/core/string_buffer.dart:31: external void write(Object obj);
On 2013/02/19 12:11:44, ahe wrote:
> You forgot to document all the new methods you added.

Documentation for them is inherited.