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

sdk/lib/io/stdio.dart

 // Copyright (c) 2013, 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.
 
 part of dart.io;
 
 const int _STDIO_HANDLE_TYPE_TERMINAL = 0;
 const int _STDIO_HANDLE_TYPE_PIPE = 1;
 const int _STDIO_HANDLE_TYPE_FILE = 2;
 const int _STDIO_HANDLE_TYPE_SOCKET = 3;
@@ skipping 27 matching lines @@
 class Stdin extends _StdStream implements Stream<List<int>> {
   Stdin._(Stream<List<int>> stream) : super(stream);
 
   /**
    * Synchronously read a line from stdin. This call will block until a full
    * line is available. The line will contain the newline character(s).
    *
    * If end-of-file is reached, `null` is returned.
    *
    * If end-of-file is reached after some data has already been read, that data
    * is returned.

[Søren Gjesse, 2014/04/22 08:34:26]

Please document the optional arguments.

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

Done.

    */
   String readLineSync({Encoding encoding: SYSTEM_ENCODING,
                        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);
   }
 
   /**
    * Check if echo mode is enabled on [stdin].
    */
   external bool get echoMode;
 
   /**
    * Enable or disable echo mode on [stdin].
    *
@@ skipping 201 matching lines @@
   return StdioType.OTHER;
 }
 
 
 class _StdIOUtils {
   external static _getStdioOutputStream(int fd);
   external static Stdin _getStdioInputStream();
   external static int _socketType(nativeSocket);
   external static _getStdioHandleType(int fd);
 }