Add optional startIndex to String.replaceFirst

runtime/lib/string_patch.dart

Index: runtime/lib/string_patch.dart
diff --git a/runtime/lib/string_patch.dart b/runtime/lib/string_patch.dart
index 11f549338be8cc80828f2aa37059c0845315c51a..1835f789caec343f124826454900b86ef7043320 100644
--- a/runtime/lib/string_patch.dart
+++ b/runtime/lib/string_patch.dart
@@ -414,23 +414,27 @@ class _StringBase {
     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) {