Make String.fromCharCodes take start/end.

Make String.fromCharCodes take start/end.

This avoids having to make a sublist of a list of character codes before passing it to String.fromCharCodes.

    new String.fromCharCodes(codes.sublist(start, end))

becomes just

    new String.fromCharCodes(codes, start, end)

R=floitsch@google.com, sgjesse@google.com

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

[Lasse Reichstein Nielsen, 2014-08-29 08:36:23]

lrn@google.com changed reviewers:
+ floitsch@google.com, sgjesse@google.com, srdjan@google.com

[floitsch, 2014-08-29 11:13:21]

LGTM.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:53: ASSERT(start <= end);
Don't assert but check.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:63: ASSERT(end <= growableArray.Length());
ditto.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:66: ASSERT(end <= a.Length());
ditto.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:177: ASSERT(length <= array.LengthInBytes());
ditto.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:181: ASSERT(length <= array.LengthInBytes());
ditto.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:185: ASSERT(length <=
Smi::Value(TypedDataView::Length(view)));
ditto.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:197: ASSERT(length <= array.Length());
ditto.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:207: ASSERT(length <= array.Length());
ditto.

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
File sdk/lib/_internal/lib/core_patch.dart (right):

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/core_patch.dart:277: if (charCodes is JSArray) {
does it need to be a JSArray?
Or can we use isJsIndexable? Would that cover the comment in line 279?

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/convert/utf.dart...
sdk/lib/convert/utf.dart:440: // if (from == 0 && to == codeUnits.length) {
dead code?

[Søren Gjesse, 2014-08-29 11:16:04]

lgtm

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string_patch...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:72: if (start < 0) start = 0;
Why is this not an ArgumentError?

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
File sdk/lib/_internal/lib/core_patch.dart (right):

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/core_patch.dart:275: if (start < 0) start = 0;
ArgumentError?

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/convert/utf.dart...
sdk/lib/convert/utf.dart:440: // if (from == 0 && to == codeUnits.length) {
Code in comment.

[Lasse Reichstein Nielsen, 2014-09-02 07:57:05]

PTAL srdjan

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:53: ASSERT(start <= end);
On 2014/08/29 11:13:20, floitsch wrote:
> Don't assert but check.

Done.

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string_patch...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/515183002/diff/80001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:72: if (start < 0) start = 0;
I'm feeling generous? :)
Probably should be an error, I'll change it.

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
File sdk/lib/_internal/lib/core_patch.dart (right):

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/core_patch.dart:275: if (start < 0) start = 0;
On 2014/08/29 11:16:03, Søren Gjesse wrote:
> ArgumentError?

Done.

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/core_patch.dart:277: if (charCodes is JSArray) {
No idea - I was going by the existing code only.

It's worth a try, but I'd keep it in a separate CL.

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/core_patch.dart:284: charCodes = list.sublist(start, end);
should be "list = ...".

https://codereview.chromium.org/515183002/diff/80001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/core_patch.dart:295: list = new List.from(charCodes);
should be .from(list);

[srdjan, 2014-09-17 15:31:59]

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string.cc#n...
runtime/lib/string.cc:53: ASSERT(start <= end);
Please explain in comment why  you cannot guarantee that list is a valid
argument, while you can guarantee that 'start' and 'end' are valid.

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

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:8: return
_StringBase.createFromCharCodes(charCodes, start, end);
Adding optional arguments will slow down this factory; please verify performance
of tracked benchmarks.

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:73: if (end == null || end > charCodes.length) {
Please use parentheses

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:100: throw new ArgumentError(charCodes);
Why not in the start:
if (charCodes == null) throw new ArgumentError(charCodes);

and avoid the nested main body.

https://codereview.chromium.org/515183002/diff/120001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/515183002/diff/120001/sdk/lib/convert/utf.dar...
sdk/lib/convert/utf.dart:445: // }
Remove dead code?

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

https://codereview.chromium.org/515183002/diff/120001/sdk/lib/core/string.dar...
sdk/lib/core/string.dart:118: [int start = 0, int end]);
I assume the library team has agreed to this API change.

[Lasse Reichstein Nielsen, 2014-09-18 09:03:01]

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string.cc#n...
runtime/lib/string.cc:53: ASSERT(start <= end);
Good point. It's probably safer to check it and throw proper ArgumentErrors.

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

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:8: return
_StringBase.createFromCharCodes(charCodes, start, end);
Will do. 

Alternatively, we could have a
  String.fromCharCodeRange(Iterable<int> charCodes, int start, int end).

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:73: if (end == null || end > charCodes.length) {
On 2014/09/17 15:31:58, srdjan wrote:
> Please use parentheses

Done.

https://codereview.chromium.org/515183002/diff/120001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:100: throw new ArgumentError(charCodes);
A naive attempt at putting the common branch first. Probably irrelevant in
practice. I'll rewrite without the nesting.

https://codereview.chromium.org/515183002/diff/120001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/515183002/diff/120001/sdk/lib/convert/utf.dar...
sdk/lib/convert/utf.dart:445: // }
On 2014/09/17 15:31:58, srdjan wrote:
> Remove dead code?

Done.

[Lasse Reichstein Nielsen, 2014-09-25 10:45:35]

Committed patchset #9 (id:160001) manually as 40672 (presubmit successful).

[sra1, 2014-09-25 19:58:24]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2014-09-25 19:58:25]

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

https://codereview.chromium.org/515183002/diff/160001/sdk/lib/core/string.dar...
sdk/lib/core/string.dart:115: * has no effect.
Most APIs with a start and end are strict, requiring 

0 <= start <= end <= length.

This API is less strict.

Is this a deliberate choice to make this API inconsistent with sublist,
substring etc?
If there is not a strong reason, I'd prefer the libraries to be consistent.

[Lasse Reichstein Nielsen, 2014-09-25 20:01:08]

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

https://codereview.chromium.org/515183002/diff/160001/sdk/lib/core/string.dar...
sdk/lib/core/string.dart:115: * has no effect.
It is deliberate because the input is an iterable, not a list.
You are not expected to know the length of an iterable, so this works as if by
charcodes.take(end).skip(start).

We should probably not have allowed an iterable to being with, only lists.

[floitsch, 2014-09-25 20:08:46]

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

https://codereview.chromium.org/515183002/diff/160001/sdk/lib/core/string.dar...
sdk/lib/core/string.dart:115: * has no effect.
On 2014/09/25 20:01:08, Lasse Reichstein Nielsen wrote:
> It is deliberate because the input is an iterable, not a list.
> You are not expected to know the length of an iterable, so this works as if by
> charcodes.take(end).skip(start).
> 
> We should probably not have allowed an iterable to being with, only lists.

I agree with Stephen: start, end should have the same meaning everywhere.

[Lasse Reichstein Nielsen, 2014-09-26 06:02:19]

On 2014/09/25 20:08:46, floitsch wrote:
> https://codereview.chromium.org/515183002/diff/160001/sdk/lib/core/string.dart
> File sdk/lib/core/string.dart (right):
> 
>
https://codereview.chromium.org/515183002/diff/160001/sdk/lib/core/string.dar...
> sdk/lib/core/string.dart:115: * has no effect.
> On 2014/09/25 20:01:08, Lasse Reichstein Nielsen wrote:
> > It is deliberate because the input is an iterable, not a list.
> > You are not expected to know the length of an iterable, so this works as if
by
> > charcodes.take(end).skip(start).
> > 
> > We should probably not have allowed an iterable to being with, only lists.
> 
> I agree with Stephen: start, end should have the same meaning everywhere.

Ok, I'll change it.

[Lasse Reichstein Nielsen, 2014-10-06 11:25:03]

Fixe din CL https://codereview.chromium.org/605213002