Improve the replaceFirst, replaceAll and split methods.

Improve the replaceFirst, replaceAll and split methods.

An empty regular expression had caused split to loop infinitely.  Now,
the split method behaves like its namesake in ECMAScript and splits a
string along character boundaries.

The split, replaceFirst, and replaceAll methods only supported RegExp
and String patterns.  Now, these methods are implemented on top of the
allMatches method and work for all subinterfaces of Pattern.

BUG=2171
Committed: https://code.google.com/p/dart/source/detail?r=5653

[Ivan Posva, 2012-03-16 21:43:57]

LGTM with comments.

-Ivan

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.dart
File runtime/lib/string.dart (right):

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.d...
runtime/lib/string.dart:278: int endIndex = position + this.length;
Want to pull this.length out in a similar fashion to str.length?

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.d...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
Can you please add a comment explaining this somewhat confusing condition?

[ngeoffray, 2012-03-17 09:43:58]

Hi Carl,

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.dart
File runtime/lib/string.dart (right):

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.d...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
[] : [this] -> <String>[] : <String>[this]?

[cshapiro, 2012-03-19 06:39:29]

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.dart
File runtime/lib/string.dart (right):

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.d...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
can you give me more context for why this might be needed?

[Ivan Posva, 2012-03-19 06:42:19]

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.dart
File runtime/lib/string.dart (right):

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.d...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
On 2012/03/19 06:39:29, cshapiro wrote:
> can you give me more context for why this might be needed?

Because the return type of the method is List<String>. I missed this in the
original code review, but look at the line below, where there is the new
List<String>().

[cshapiro, 2012-03-19 06:51:45]

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.dart
File runtime/lib/string.dart (right):

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.d...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
I am curious to know if some combination of tools or runtime checking tripped
over this.  I am happy to add the checks, even if it is only on principle, but
it would be useful to know what might break or complain when there is a similar
disagreement (presumably, in this case "List<Dynamic> <: List<String>" does not
hold).

[ngeoffray, 2012-03-19 08:25:38]

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.dart
File runtime/lib/string.dart (right):

https://chromiumcodereview.appspot.com/9689089/diff/5001/runtime/lib/string.d...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
On 2012/03/19 06:51:45, cshapiro wrote:
> I am curious to know if some combination of tools or runtime checking tripped
> over this.  I am happy to add the checks, even if it is only on principle, but
> it would be useful to know what might break or complain when there is a
similar
> disagreement (presumably, in this case "List<Dynamic> <: List<String>" does
not
> hold).

If you return just a list, the VM will not complain when you add something
different than a string to the list. As a user of split, I want the return value
to be of List<String> so that, if I do any mistake when adding things to the
list, the VM in checked mode throws an exception.

Also I believe it should help the VM in checked mode for not making any String
check when fetching something from a List<String>.

[cshapiro, 2012-03-19 17:50:55]

http://codereview.chromium.org/9689089/diff/5001/runtime/lib/string.dart
File runtime/lib/string.dart (right):

http://codereview.chromium.org/9689089/diff/5001/runtime/lib/string.dart#newc...
runtime/lib/string.dart:278: int endIndex = position + this.length;
Good idea.  Done.

http://codereview.chromium.org/9689089/diff/5001/runtime/lib/string.dart#newc...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
Sure, done.  I have simplified the condition somewhat as well.

http://codereview.chromium.org/9689089/diff/5001/runtime/lib/string.dart#newc...
runtime/lib/string.dart:294: return iterator.hasNext() ? [] : [this];
Got it.  Thanks.

[ngeoffray, 2012-03-19 20:13:29]

Hi Carl,

http://codereview.chromium.org/9689089/diff/11001/tests/corelib/src/StringPat...
File tests/corelib/src/StringPatternTest.dart (right):

http://codereview.chromium.org/9689089/diff/11001/tests/corelib/src/StringPat...
tests/corelib/src/StringPatternTest.dart:61:
Expect.isTrue(matches.iterator().hasNext());
I believe one of these changes broke the leg bot.

[ngeoffray, 2012-03-19 20:14:15]

http://codereview.chromium.org/9689089/diff/11001/tests/corelib/src/StringPat...
File tests/corelib/src/StringPatternTest.dart (right):

http://codereview.chromium.org/9689089/diff/11001/tests/corelib/src/StringPat...
tests/corelib/src/StringPatternTest.dart:61:
Expect.isTrue(matches.iterator().hasNext());
On 2012/03/19 20:13:29, ngeoffray wrote:
> I believe one of these changes broke the leg bot.

%s/leg/dart2js