Improve the replaceFirst, replaceAll and split methods.

runtime/lib/string.dart

 // Copyright (c) 2011, 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.
 
 /**
  * [StringBase] contains common methods used by concrete String implementations,
  * e.g., OneByteString.
  */
 class StringBase {
 
@@ skipping 173 matching lines @@
     }
     if ((first == 0) && (last == (len - 1))) {
       // Returns this string if it does not have leading or trailing
       // whitespaces.
       return this;
     } else {
       return substringUnchecked_(first, last + 1);
     }
   }
 
-  bool contains(Pattern other, [int startIndex = 0]) {
-    if (other is String) {
-      return indexOf(other, startIndex) >= 0;
+  bool contains(Pattern pattern, [int startIndex = 0]) {
+    if (pattern is String) {
+      return indexOf(pattern, startIndex) >= 0;
     }
-    return other.allMatches(this.substring(startIndex)).iterator().hasNext();
+    return pattern.allMatches(this.substring(startIndex)).iterator().hasNext();
   }
 
-  String replaceFirst(Pattern pattern, String to) {
-    if (pattern is RegExp) {
-      StringBuffer buffer = new StringBuffer();
-      int startIndex = 0;
-      Match match = pattern.firstMatch(this);
-      if (match != null) {
-        buffer.add(this.substring(startIndex, match.start())).add(to);
-        startIndex = match.end();
-      }
-      return buffer.add(this.substring(startIndex)).toString();
+  String replaceFirst(Pattern pattern, String replacement) {
+    if (pattern is! Pattern) {
+      throw new IllegalArgumentException("${pattern} is not a Pattern");
     }
-    int pos = this.indexOf(pattern, 0);
-    if (pos < 0) {
-      return this;
+    if (replacement is! String) {
+      throw new IllegalArgumentException("${replacement} is not a String");
     }
-    String s1 = this.substring(0, pos);
-    String s2 = this.substring(pos + pattern.length, this.length);
-    return s1.concat(to.concat(s2));
+    StringBuffer buffer = new StringBuffer();
+    int startIndex = 0;
+    Iterator iterator = pattern.allMatches(this).iterator();
+    if (iterator.hasNext()) {
+      Match match = iterator.next();
+      buffer.add(this.substring(startIndex, match.start())).add(replacement);
+      startIndex = match.end();
+    }
+    return buffer.add(this.substring(startIndex)).toString();
   }
 
-  String replaceAll(Pattern pattern, String to) {
-    if (pattern is RegExp) {
-      StringBuffer buffer = new StringBuffer();
-      int startIndex = 0;
-      for (Match match in pattern.allMatches(this)) {
-        buffer.add(this.substring(startIndex, match.start())).add(to);
-        startIndex = match.end();
-      }
-      return buffer.add(this.substring(startIndex)).toString();
+  String replaceAll(Pattern pattern, String replacement) {
+    if (pattern is! Pattern) {
+      throw new IllegalArgumentException("${pattern} is not a Pattern");
     }
-    String from = pattern;
-    int fromLength = from.length;
-    int toLength = to.length;
-    int thisLength = this.length;
-
-    StringBuffer result = new StringBuffer("");
-    // Special case the empty string replacement where [to] is
-    // inserted in between each character.
-    if (fromLength === 0) {
-      result.add(to);
-      for (int i = 0; i < thisLength; i++) {
-        result.add(this.substring(i, i + 1));
-        result.add(to);
-      }
-      return result.toString();
+    if (replacement is! String) {
+      throw new IllegalArgumentException("${replacement} is not a String");
     }
-
-    int index = indexOf(from, 0);
-    if (index < 0) {
-      return this;
+    StringBuffer buffer = new StringBuffer();
+    int startIndex = 0;
+    for (Match match in pattern.allMatches(this)) {
+      buffer.add(this.substring(startIndex, match.start())).add(replacement);
+      startIndex = match.end();
     }
-    int startIndex = 0;
-    do {
-      result.add(this.substring(startIndex, index));
-      result.add(to);
-      startIndex = index + fromLength;
-    } while ((index = indexOf(from, startIndex)) >= 0);
-
-    // If there are remaining code points, add them to the string
-    // buffer.
-    if (startIndex < thisLength) {
-      result.add(this.substring(startIndex, thisLength));
-    }
-
-    return result.toString();
+    return buffer.add(this.substring(startIndex)).toString();
   }
 
   /**
    * Convert argument obj to string and concat it with this string.
    * Returns concatenated string.
    */
   String operator +(Object obj) {
     return this.concat(obj.toString());
   }
 
@@ skipping 17 matching lines @@
       int strLength = str.length;
       for (int k = 0; k < strLength; k++) {
         codepoints[intArrayIx++] = str.charCodeAt(k);
       }
     }
     return StringBase.createFromCharCodes(codepoints);
   }
 
   Iterable<Match> allMatches(String str) {
     List<Match> result = new List<Match>();
-    if (this.isEmpty()) return result;
-    int length = this.length;
-
-    int ix = 0;
-    while (ix < str.length) {
-      int foundIx = str.indexOf(this, ix);
-      if (foundIx < 0) break;
-      result.add(new _StringMatch(foundIx, str, this));
-      ix = foundIx + length;
+    int length = str.length;
+    int startIndex = 0;
+    while (true) {
+      int position = str.indexOf(this, startIndex);
+      if (position == -1) {
+        break;
+      }
+      result.add(new _StringMatch(position, str, this));
+      int endIndex = position + this.length;

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

Want to pull this.length out in a similar fashion to str.length?

[cshapiro, 2012/03/19 17:50:56]

Good idea.  Done.

+      if (endIndex == length) {
+        break;
+      } else if (position == endIndex) {
+        ++startIndex;  // empty match, advance and restart
+      } else {
+        startIndex = endIndex;
+      }
     }
     return result;
   }
 
   List<String> split(Pattern pattern) {
+    int length = this.length;
+    Iterator iterator = pattern.allMatches(this).iterator();
+    if (length == 0) {
+      return iterator.hasNext() ? [] : [this];

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

Can you please add a comment explaining this somewhat confusing condition?

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

[] : [this] -> <String>[] : <String>[this]?

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

can you give me more context for why this might be needed?

[Ivan Posva, 2012/03/19 06:42:20]

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]

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]

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:56]

Sure, done.  I have simplified the condition somewhat as well.

[cshapiro, 2012/03/19 17:50:56]

Got it.  Thanks.

+    }
     List<String> result = new List<String>();
-    if (pattern is RegExp) {
-      int startIndex = 0;
-      for (Match match in pattern.allMatches(this)) {
-        result.add(this.substring(startIndex, match.start()));
-        startIndex = match.end();
-      }
-      result.add(this.substring(startIndex));
-      return result;
-    }
-    if (pattern.isEmpty()) {
-      for (int i = 0; i < this.length; i++) {
-        result.add(this.substring(i, i+1));
-      }
-      return result;
-    }
-    int ix = 0;
-    while (ix < this.length) {
-      int foundIx = this.indexOf(pattern, ix);
-      if (foundIx < 0) {
-        // Not found, add remaining.
-        result.add(this.substring(ix, this.length));
+    int startIndex = 0;
+    int previousIndex = 0;
+    while (true) {
+      if (startIndex == length || !iterator.hasNext()) {
+        result.add(this.substring(previousIndex, length));
         break;
       }
-      result.add(this.substring(ix, foundIx));
-      ix = foundIx + pattern.length;
-    }
-    if (ix == this.length) {
-      result.add("");
+      Match match = iterator.next();
+      if (match.start() == length) {
+        result.add(this.substring(previousIndex, length));
+        break;
+      }
+      var endIndex = match.end();
+      if (startIndex == endIndex && endIndex == previousIndex) {
+        ++startIndex;  // empty match, advance and restart
+        continue;
+      }
+      result.add(this.substring(previousIndex, match.start()));
+      startIndex = previousIndex = endIndex;
     }
     return result;
   }
 
   List<String> splitChars() {
     int len = this.length;
     final result = new List<String>(len);
     for (int i = 0; i < len; i++) {
       result[i] = this[i];
     }
@@ skipping 68 matching lines @@
   }
 
 }
 
 
 class TwoByteString extends StringBase implements String {
   factory TwoByteString._uninstantiable() {
     throw const UnsupportedOperationException(
         "TwoByteString can only be allocated by the VM");
   }
-  
+
   // Checks for one-byte whitespaces only.
   // TODO(srdjan): Investigate if 0x85 (NEL) and 0xA0 (NBSP) are valid
   // whitespaces. Add checking for multi-byte whitespace codepoints.
   bool _isWhitespace(int codePoint) {
     return
       (codePoint === 32) || // Space.
       ((9 <= codePoint) && (codePoint <= 13)); // CR, LF, TAB, etc.
   }
 }
 
 
 class FourByteString extends StringBase implements String {
   factory FourByteString._uninstantiable() {
     throw const UnsupportedOperationException(
         "FourByteString can only be allocated by the VM");
   }
-  
+
   // Checks for one-byte whitespaces only.
   // TODO(srdjan): Investigate if 0x85 (NEL) and 0xA0 (NBSP) are valid
   // whitespaces. Add checking for multi-byte whitespace codepoints.
   bool _isWhitespace(int codePoint) {
     return
       (codePoint === 32) || // Space.
       ((9 <= codePoint) && (codePoint <= 13)); // CR, LF, TAB, etc.
   }
 }
 
 
 class ExternalOneByteString extends StringBase implements String {
   factory ExternalOneByteString._uninstantiable() {
     throw const UnsupportedOperationException(
         "ExternalOneByteString can only be allocated by the VM");
   }
-  
+
   // Checks for one-byte whitespaces only.
   // TODO(srdjan): Investigate if 0x85 (NEL) and 0xA0 (NBSP) are valid
   // whitespaces for one byte strings.
   bool _isWhitespace(int codePoint) {
     return
       (codePoint === 32) || // Space.
       ((9 <= codePoint) && (codePoint <= 13)); // CR, LF, TAB, etc.
   }
 }
 
 
 class ExternalTwoByteString extends StringBase implements String {
   factory ExternalTwoByteString._uninstantiable() {
     throw const UnsupportedOperationException(
         "ExternalTwoByteString can only be allocated by the VM");
   }
-  
+
   // Checks for one-byte whitespaces only.
   // TODO(srdjan): Investigate if 0x85 (NEL) and 0xA0 (NBSP) are valid
   // whitespaces. Add checking for multi-byte whitespace codepoints.
   bool _isWhitespace(int codePoint) {
     return
       (codePoint === 32) || // Space.
       ((9 <= codePoint) && (codePoint <= 13)); // CR, LF, TAB, etc.
   }
 }
 
 
 class ExternalFourByteString extends StringBase implements String {
   factory ExternalFourByteString._uninstantiable() {
     throw const UnsupportedOperationException(
         "ExternalFourByteString can only be allocated by the VM");
   }
-  
+
   // Checks for one-byte whitespaces only.
   // TODO(srdjan): Investigate if 0x85 (NEL) and 0xA0 (NBSP) are valid
   // whitespaces. Add checking for multi-byte whitespace codepoints.
   bool _isWhitespace(int codePoint) {
     return
       (codePoint === 32) || // Space.
       ((9 <= codePoint) && (codePoint <= 13)); // CR, LF, TAB, etc.
   }
 }
 
@@ skipping 20 matching lines @@
     for (int g in groups) {
       result.add(group(g));
     }
     return result;
   }
 
   final int _start;
   final String str;
   final String pattern;
 }