Reinstall previous behavior for Set and Queue toString.

sdk/lib/collection/iterable.dart

Index: sdk/lib/collection/iterable.dart
diff --git a/sdk/lib/collection/iterable.dart b/sdk/lib/collection/iterable.dart
index b6e52f4669e931320a443c262cd2208ca9668770..e4352743064b728c562d85d31cff883ee467fecf 100644
--- a/sdk/lib/collection/iterable.dart
+++ b/sdk/lib/collection/iterable.dart
@@ -195,7 +195,7 @@ abstract class IterableMixin<E> implements Iterable<E> {
     throw new RangeError.value(index);
   }
 
-  String toString() => _iterableToString(this);
+  String toString() => IterableBase.iterableToShortString(this, '(', ')');
 }
 
 /**
@@ -402,138 +402,185 @@ abstract class IterableBase<E> implements Iterable<E> {
    * included from the start of the iterable.
    *
    * The conversion may omit calling `toString` on some elements if they
-   * are known to now occur in the output, and it may stop iterating after
+   * are known to not occur in the output, and it may stop iterating after
    * a hundred elements.
    */
-  String toString() => _iterableToString(this);
-}
-
-String _setToString(Set set) => _collectionToString(set, "{" , "}");
+  String toString() => IterableBase.iterableToShortString(this, '(', ')');

[floitsch, 2014/05/23 16:09:58]

-IterableBase.-

[Lasse Reichstein Nielsen, 2014/05/23 16:50:01]

You mean I should remove it?
It is in the same class now, so I guess it's safe.

 
-String _iterableToString(Iterable iterable) =>
-    _collectionToString(iterable, "(", ")");
+  /**
+   * Convert an `Iterable` to a string like [IterableBase.toString].
+   *
+   * Allows using other delimiters than '(' and ')'.
+   *
+   * Handles circular references where converting one of the elements
+   * to a string ends up converting [iterable] to a string again.
+   */
+  static String iterableToShortString(Iterable iterable,

[floitsch, 2014/05/23 16:09:58]

Isn't this an exposed class?
Do you want to expose "iterableToShortString" ?

[Lasse Reichstein Nielsen, 2014/05/23 16:50:01]

Yes, I want to expose it.
I see no reason to have perfectly good functionality unexposed.
We think it's a reasonable default toString on Iterable, but we haven't made it
possible to use it on a Set or List, even if you wanted it (well, you could wrap
it in something that exposed the Set's iterator and used IterableMixin's
toString, but there is no easy way).

This way a user can decide to have a short, but still cycle-safe, toString of
their Set, like the default on IterableBase.

And no, the name isn't perfect. It's intended for programmers wanting to
implement some sub-type of Iterable, so it'll likely be hidden behind a more
meaningfull name (like toString). If you want to use it for debugging, that's
cool too.

+                                      [String leftDelimiter = '(',
+                                       String rightDelimiter = ')']) {
+    if (_toStringVisiting.contains(iterable)) {
+      if (leftDelimiter == "(" && rightDelimiter == ")") {
+        // Avoid creating a new string in the "common" case.
+        return "(...)";
+      }
+      return "$leftDelimiter...$rightDelimiter";
+    }
+    List parts = [];
+    _toStringVisiting.add(iterable);
+    try {
+      _iterablePartsToStrings(iterable, parts);
+    } finally {
+      _toStringVisiting.remove(iterable);
+    }
+    return (new StringBuffer(leftDelimiter)
+                ..writeAll(parts, ", ")
+                ..write(rightDelimiter)).toString();
+  }
 
-String _collectionToString(Iterable iterable, String before, String after) {
-  if (_toStringVisiting.contains(iterable)) return "$before...$after";
-  _toStringVisiting.add(iterable);
-  List parts = [];
-  try {
-    _collectionPartsToStrings(iterable, parts);
-  } finally {
-    _toStringVisiting.remove(iterable);
+  /**
+   * Converts an `Iterable` to a string.
+   *
+   * Converts each elements to a string, and separates the results by ", ".
+   * Then wraps the result in [leftDelimiter] and [rightDelimiter].
+   *
+   * Unlike [iterableToShortString], this conversion doesn't omit any
+   * elements or puts any limit on the size of the result.
+   *
+   * Handles circular references where converting one of the elements
+   * to a string ends up converting [iterable] to a string again.
+   */
+  static String iterableToFullString(Iterable iterable,

[floitsch, 2014/05/23 16:09:58]

ditto.

[Lasse Reichstein Nielsen, 2014/05/23 16:50:01]

And yes, intended public.
I don't want anybody to ever cook up their own toString over a subclass of
Iterable and forget to handle cycles.

+                                     [String leftDelimiter = '(',
+                                      String rightDelimiter = ')']) {
+    if (_toStringVisiting.contains(iterable)) {
+      return "$leftDelimiter...$rightDelimiter";
+    }
+    StringBuffer buffer = new StringBuffer(leftDelimiter);
+    _toStringVisiting.add(iterable);
+    try {
+      buffer.writeAll(iterable, ", ");
+    } finally {
+      _toStringVisiting.remove(iterable);
+    }
+    buffer.write(rightDelimiter);
+    return buffer.toString();
   }
-  return (new StringBuffer(before)
-              ..writeAll(parts, ", ")
-              ..write(after)).toString();
-}
 
-/** Convert elments of [iterable] to strings and store them in [parts]. */
-void _collectionPartsToStrings(Iterable iterable, List parts) {
-  /// Try to stay below this many characters.
-  const int LENGTH_LIMIT = 80;
-  /// Always at least this many elements at the start.
-  const int HEAD_COUNT = 3;
-  /// Always at least this many elements at the end.
-  const int TAIL_COUNT = 2;
-  /// Stop iterating after this many elements. Iterables can be infinite.
-  const int MAX_COUNT = 100;
-  // Per entry length overhead. It's for ", " for all after the first entry,
-  // and for "(" and ")" for the initial entry. By pure luck, that's the same
-  // number.
-  const int OVERHEAD = 2;
-  const int ELLIPSIS_SIZE = 3;  // "...".length.
-  int length = 0;
-  int count = 0;
-  Iterator it = iterable.iterator;
-  // Initial run of elements, at least HEAD_COUNT, and then continue until
-  // passing at most LENGTH_LIMIT characters.
-  while (length < LENGTH_LIMIT || count < HEAD_COUNT) {
-    if (!it.moveNext()) {
-      return;
-    }
-    String next = "${it.current}";
-    parts.add(next);
-    length += next.length + OVERHEAD;
-    count++;
-  }
-
-  String penultimateString;
-  String ultimateString;
-
-  // Find last two elements. One or more of them may already be in the
-  // parts array. Include their length in `length`.
-  var penultimate = null;
-  var ultimate = null;
-  if (!it.moveNext()) {
-    if (count <= HEAD_COUNT + TAIL_COUNT) return;
-    ultimateString = parts.removeLast();
-    penultimateString = parts.removeLast();
-  } else {
-    penultimate = it.current;
-    count++;
+  /** A set used to identify cyclic lists during toString() calls. */
+  static Set _toStringVisiting = new HashSet.identity();
+
+  /**
+   * Convert elments of [iterable] to strings and store them in [parts].
+   *
+   * This is the complicated part of [iterableToShortString].
+   * It is extracted as a separate function to avoid having too much code
+   * inside the try/finally.

[floitsch, 2014/05/23 16:09:58]

This should not be a dart-doc comment.

Feel free to leave it as a normal comment below the dart-doc, though.

[Lasse Reichstein Nielsen, 2014/05/23 16:50:01]

Why not. It's a private method, so it won't be exposed on api.dartlang.org, but
I sure hope the editor will show it as tooltip on the call, if you are editing
this file.

+   */
+  static void _iterablePartsToStrings(Iterable iterable, List parts) {
+    /// Try to stay below this many characters.
+    const int LENGTH_LIMIT = 80;
+    /// Always at least this many elements at the start.
+    const int HEAD_COUNT = 3;
+    /// Always at least this many elements at the end.
+    const int TAIL_COUNT = 2;
+    /// Stop iterating after this many elements. Iterables can be infinite.
+    const int MAX_COUNT = 100;
+    // Per entry length overhead. It's for ", " for all after the first entry,
+    // and for "(" and ")" for the initial entry. By pure luck, that's the same
+    // number.
+    const int OVERHEAD = 2;
+    const int ELLIPSIS_SIZE = 3;  // "...".length.
+
+    int length = 0;
+    int count = 0;
+    Iterator it = iterable.iterator;
+    // Initial run of elements, at least HEAD_COUNT, and then continue until
+    // passing at most LENGTH_LIMIT characters.
+    while (length < LENGTH_LIMIT || count < HEAD_COUNT) {
+      if (!it.moveNext()) return;
+      String next = "${it.current}";
+      parts.add(next);
+      length += next.length + OVERHEAD;
+      count++;
+    }
+
+    String penultimateString;
+    String ultimateString;
+
+    // Find last two elements. One or more of them may already be in the
+    // parts array. Include their length in `length`.
+    var penultimate = null;
+    var ultimate = null;
     if (!it.moveNext()) {
-      if (count <= HEAD_COUNT + 1) {
-        parts.add("$penultimate");
-        return;
-      }
-      ultimateString = "$penultimate";
+      if (count <= HEAD_COUNT + TAIL_COUNT) return;
+      ultimateString = parts.removeLast();
       penultimateString = parts.removeLast();
-      length += ultimateString.length + OVERHEAD;
     } else {
-      ultimate = it.current;
+      penultimate = it.current;
       count++;
-      // Then keep looping, keeping the last two elements in variables.
-      assert(count < MAX_COUNT);
-      while (it.moveNext()) {
-        penultimate = ultimate;
+      if (!it.moveNext()) {
+        if (count <= HEAD_COUNT + 1) {
+          parts.add("$penultimate");
+          return;
+        }
+        ultimateString = "$penultimate";
+        penultimateString = parts.removeLast();
+        length += ultimateString.length + OVERHEAD;
+      } else {
         ultimate = it.current;
         count++;
-        if (count > MAX_COUNT) {
-          // If we haven't found the end before MAX_COUNT, give up.
-          // This cannot happen in the code above because each entry
-          // increases length by at least two, so there is no way to
-          // visit more than ~40 elements before this loop.
-
-          // Remove any surplus elements until length, including ", ...)",
-          // is at most LENGTH_LIMIT.
-          while (length > LENGTH_LIMIT - ELLIPSIS_SIZE - OVERHEAD &&
-                 count > HEAD_COUNT) {
-            length -= parts.removeLast().length + OVERHEAD;
-            count--;
+        // Then keep looping, keeping the last two elements in variables.
+        assert(count < MAX_COUNT);
+        while (it.moveNext()) {
+          penultimate = ultimate;
+          ultimate = it.current;
+          count++;
+          if (count > MAX_COUNT) {
+            // If we haven't found the end before MAX_COUNT, give up.
+            // This cannot happen in the code above because each entry
+            // increases length by at least two, so there is no way to
+            // visit more than ~40 elements before this loop.
+
+            // Remove any surplus elements until length, including ", ...)",
+            // is at most LENGTH_LIMIT.
+            while (length > LENGTH_LIMIT - ELLIPSIS_SIZE - OVERHEAD &&
+                   count > HEAD_COUNT) {
+              length -= parts.removeLast().length + OVERHEAD;
+              count--;
+            }
+            parts.add("...");
+            return;
           }
-          parts.add("...");
-          return;
         }
+        penultimateString = "$penultimate";
+        ultimateString = "$ultimate";
+        length +=
+            ultimateString.length + penultimateString.length + 2 * OVERHEAD;
       }
-      penultimateString = "$penultimate";
-      ultimateString = "$ultimate";
-      length +=
-          ultimateString.length + penultimateString.length + 2 * OVERHEAD;
     }
-  }
-
-  // If there is a gap between the initial run and the last two,
-  // prepare to add an ellipsis.
-  String elision = null;
-  if (count > parts.length + TAIL_COUNT) {
-    elision = "...";
-    length += ELLIPSIS_SIZE + OVERHEAD;
-  }
 
-  // If the last two elements were very long, and we have more than
-  // HEAD_COUNT elements in the initial run, drop some to make room for
-  // the last two.
-  while (length > LENGTH_LIMIT && parts.length > HEAD_COUNT) {
-    String lastPart = parts.removeLast();
-    length -= lastPart.length + OVERHEAD;
-    if (elision == null) {
+    // If there is a gap between the initial run and the last two,
+    // prepare to add an ellipsis.
+    String elision = null;
+    if (count > parts.length + TAIL_COUNT) {
       elision = "...";
       length += ELLIPSIS_SIZE + OVERHEAD;
     }
+
+    // If the last two elements were very long, and we have more than
+    // HEAD_COUNT elements in the initial run, drop some to make room for
+    // the last two.
+    while (length > LENGTH_LIMIT && parts.length > HEAD_COUNT) {
+      length -= parts.removeLast().length + OVERHEAD;
+      if (elision == null) {
+        elision = "...";
+        length += ELLIPSIS_SIZE + OVERHEAD;
+      }
+    }
+    if (elision != null) {
+      parts.add(elision);
+    }
+    parts.add(penultimateString);
+    parts.add(ultimateString);
   }
-  if (elision != null) {
-    parts.add(elision);
-  }
-  parts.add(penultimateString);
-  parts.add(ultimateString);
 }