Make Stdin:readLineSync not use Streams and fix special case when Windows & disabled lineMode.

sdk/lib/io/stdio.dart

Index: sdk/lib/io/stdio.dart
diff --git a/sdk/lib/io/stdio.dart b/sdk/lib/io/stdio.dart
index 9c034f437adbca9db958341dea7552cd361eaf79..d0fe2c88ca349aa16520400c5ea5fedf16a4d161 100644
--- a/sdk/lib/io/stdio.dart
+++ b/sdk/lib/io/stdio.dart
@@ -51,73 +51,58 @@ class Stdin extends _StdStream implements Stream<List<int>> {
                        bool retainNewlines: false}) {

[Lasse Reichstein Nielsen, 2014/04/22 08:07:01]

The retainNewlines parameter isn't documented.
It seems to conflict with the "The line will contain the newline characters(s)"
above.

[Anders Johnsen, 2014/04/22 10:45:42]

Done.

     const CR = 13;
     const LF = 10;
-    var line = new StringBuffer();
-    bool end = false;
+    final List line = [];
+    // On Windows, if lineMode is disabled, only CR is received.
+    bool crIsNewline = Platform.isWindows && !lineMode;
     bool lastCharWasCR = false;
-    var error;
-
-    StreamController<List<int>> controller =
-        new StreamController<List<int>>(sync: true);
-    Stream stream = controller.stream.transform(encoding.decoder);
-    stream.listen((String str) {
-      line.write(str);
-    }, onError: (e) {
-      error = e;
-    }, onDone: () {
-      end = true;
-    });
-
     bool empty = true;
-    while (!end) {
+    while (true) {
       int b = readByteSync();

[Lasse Reichstein Nielsen, 2014/04/22 08:07:01]

b -> byte

[Anders Johnsen, 2014/04/22 10:45:42]

Done.

-
       if (b < 0) {
         // We didn't write the carriage return in case a line feed would be
         // the next character. Add it now.
-        if (lastCharWasCR && !retainNewlines) controller.add([CR]);
-        controller.close();
+        if (lastCharWasCR && !retainNewlines) line.add(CR);
+        break;
       } else {
         empty = false;
         // We consider \r\n and \n as new lines.

[Lasse Reichstein Nielsen, 2014/04/22 08:07:01]

Both \r\n and \n are considered line terminators.

Consider using CR and LF instead of \r and \n, or use "\r", "\n" and "\r\n".
Escapes outside of strings looks wrong to me.

[Anders Johnsen, 2014/04/22 10:45:42]

Done.

         // A \r on its own is treated like a normal character.

[Lasse Reichstein Nielsen, 2014/04/22 08:07:01]

treated like -> treated as

[Anders Johnsen, 2014/04/22 10:45:42]

Done.

 
         if (b == CR) {
+          if (crIsNewline) {
+            if (retainNewlines) line.add(b);
+            break;
+          }
           if (lastCharWasCR && !retainNewlines) {

[Lasse Reichstein Nielsen, 2014/04/22 08:07:01]

You are testing retainNewlines twice.
How about:

if (retainNewlines) {
  line.add(b);
} else {
  if (lastCharWasCR) line.add(CR);
}
lastCharWasCR = true;

I htink retainNewlines is more stable than lastCharWasCR, so test that first.
Better branch predictability.

[Anders Johnsen, 2014/04/22 10:45:42]

Done.

             // We didn't write the carriage return in case a line feed would be
             // the next character.
             // Add it now (since we treat it like a normal character now).
-            controller.add([CR]);
+            line.add(CR);
           }
           // We add the carriage return only if we keep new lines.
           // Otherwise we need to wait for the next character (in case it is
           // a line feed).
-          if (retainNewlines) controller.add([b]);
+          if (retainNewlines) line.add(b);
           lastCharWasCR = true;
         } else if (b == LF) {
-          end = true;
           // We don't care if there was a carriage return before. If we keep
           // the line separators it has already been added to the controller.
           // Otherwise we don't want it anyway.
-          if (retainNewlines) controller.add([b]);
-          controller.close();
+          if (retainNewlines) line.add(b);
+          break;
         } else {
           // Since the current character is not a line feed we flush the
           // carriage return we didn't write last iteration.
           if (lastCharWasCR) {

[Lasse Reichstein Nielsen, 2014/04/22 08:07:01]

&& !retainNewLines  
?

[Anders Johnsen, 2014/04/22 10:45:42]

Done.

-            controller.add([CR]);
+            line.add(CR);
             lastCharWasCR = false;
           }
-          controller.add([b]);
+          line.add(b);
         }
       }
-      if (error != null) {
-        // Error during decoding.
-        throw error;
-      }
     }

[Lasse Reichstein Nielsen, 2014/04/22 08:07:01]

This seems complicated. You are handling two separate cases with a flag (or
more) that is tested all over the place.
Am I correct that if retainNewlines is true, the lastCharWasCR is unnecessary?
Consider having two different implementations, so the simple version don't get
the overhead of checking.

Also, a CR is only special before a LF, so consider having an extra loop inside
the CR case (for non-retain-newlines):

while (true) {
  if (byte == CR) {
   do {
     byte = readByteSync();
     if (byte == LF) break;
     empty = false;
     line.add(CR);
   } while (byte == CR);
  } else if (byte == LF) { 
    break;
  }
  empty = false
  line.add(byte);
}

And the code for retaining newlines would be:

var byte;
do {
  byte = readByteSync();
  line.add(byte);
} while (byte != LF)

That seems simpler than interleaving the two algorithms.

[Anders Johnsen, 2014/04/22 10:45:42]

Done.

-
     if (empty) return null;
-    return line.toString();
+    return encoding.decode(line);
   }
 
   /**