Add optional startIndex to String.replaceFirst

runtime/lib/string_patch.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 patch class String {
   /* patch */ factory String.fromCharCodes(Iterable<int> charCodes) {
     return _StringBase.createFromCharCodes(charCodes);
   }
 
   /* patch */ factory String.fromCharCode(int charCode) {
@@ skipping 396 matching lines @@
   bool contains(Pattern pattern, [int startIndex = 0]) {
     if (pattern is String) {
       if (startIndex < 0 || startIndex > this.length) {
         throw new RangeError.range(startIndex, 0, this.length);
       }
       return indexOf(pattern, startIndex) >= 0;
     }
     return pattern.allMatches(this.substring(startIndex)).isNotEmpty;
   }
 
-  String replaceFirst(Pattern pattern, String replacement) {
+  String replaceFirst(Pattern pattern, String replacement, [int startIndex = 0]) {

[Lasse Reichstein Nielsen, 2014/08/13 08:35:09]

long line.

[srawlins, 2014/08/14 00:42:44]

Done.

     if (pattern is! Pattern) {
       throw new ArgumentError("${pattern} is not a Pattern");
     }
     if (replacement is! String) {
       throw new ArgumentError("${replacement} is not a String");
     }
+    if ((startIndex < 0) || (startIndex > this.length)) {
+      throw new RangeError.value(startIndex);

[Lasse Reichstein Nielsen, 2014/08/13 08:35:09]

You can use
  throw new RangeError.range(startIndex, 0, this.length);

[srawlins, 2014/08/14 00:42:44]

Done.

+    }
     StringBuffer buffer = new StringBuffer();

[Bill Hesse, 2014/08/11 07:47:47]

This could be made more efficient by moving the StringBuffer and prefix write
inside the iterator.moveNext() check.  If there is no match, just return the
original string.  It is too bad that allMatches doesn't allow a start index.

[srawlins, 2014/08/14 00:42:44]

Done.

-    int startIndex = 0;
-    Iterator iterator = pattern.allMatches(this).iterator;
+    buffer.write(this.substring(0, startIndex));
+    String remaining = this.substring(startIndex);

[Lasse Reichstein Nielsen, 2014/08/11 08:13:32]

Creating a substring, and then using allMatches, is far too inefficient. If we
can't do it better, I'd prefer to not do it at all.

Now, what I would have want, and probably can't have, is to add startIndex as an
optional argument to Pattern.allMatches.

It should, technically, be a non-breaking change, but it can still be hard to
get changes to fundamental code through reviews.

[Lasse Reichstein Nielsen, 2014/08/11 09:37:11]

This implementation is actually wrong: A "^" must only match at the start of the
string. By using a substring, it may now match at the startIndex instead:

  "abcdefabcdef".replaceFirst(new RegExp("^a"), "b", 6)

should NOT replace the second "a".

[Lasse Reichstein Nielsen, 2014/08/13 08:35:09]

And now we DO have start-index on allMatches, so this could be just:

  Iterator iterator =   
     startIndex == 0 ? pattern.allMatches(this)
                     : pattern.allMatches(this, startIndex);
  if (!iterator.moveNext()) return this;
  Match match = iterator.current;
  return "${this.substring(0,
match.start)}$replacement${this.substring(match.end)}";
  
The special casing of startIndex == 0 is important to avoid breaking
implementations of pattern that doesn't have the second argument. Existing
programs using those would also not pass an extra argument to this method.

[srawlins, 2014/08/14 00:42:44]

Done. Thanks very much for this cleanup!

[srawlins, 2014/08/14 00:42:44]

Good catch. This implementation now depends on passing startIndex to
Regexp.allMatches, so that should cover this case. I'm adding a test as well.

[srawlins, 2014/08/14 00:42:44]

Thanks for the patch to Pattern.allMatches! This is better now (no "String
remaining").

+    Iterator iterator = pattern.allMatches(remaining).iterator;
     if (iterator.moveNext()) {
       Match match = iterator.current;
-      buffer..write(this.substring(startIndex, match.start))
+      buffer..write(remaining.substring(0, match.start))
             ..write(replacement);
       startIndex = match.end;
     }
-    return (buffer..write(this.substring(startIndex))).toString();
+    return (buffer..write(remaining.substring(startIndex))).toString();
   }
 
   String replaceAll(Pattern pattern, String replacement) {
     if (pattern is! Pattern) {
       throw new ArgumentError("${pattern} is not a Pattern");
     }
     if (replacement is! String) {
       throw new ArgumentError(
           "${replacement} is not a String or Match->String function");
     }
@@ skipping 560 matching lines @@
 class _CodeUnits extends Object with ListMixin<int>,
                                      UnmodifiableListMixin<int> {
   /** The string that this is the code units of. */
   String _string;
 
   _CodeUnits(this._string);
 
   int get length => _string.length;
   int operator[](int i) => _string.codeUnitAt(i);
 }