Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
246973002:
Make Stdin:readLineSync not use Streams and fix special case when Windows & disabled lineMode. (Closed)
|
Created:
6 years, 8 months ago by Anders Johnsen Modified:
6 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMake Stdin:readLineSync not use Streams and fix special case when Windows & disabled lineMode.
On Windows, disabled lineMode will not send any \n only \r on newlines.
BUG=http://code.google.com/p/dart/issues/detail?id=18283
R=lrn@google.com, sgjesse@google.com
Committed: https://code.google.com/p/dart/source/detail?r=35253
Patch Set 1 #
Total comments: 16
Patch Set 2 : Rewrier #
Total comments: 6
Patch Set 3 : #Messages
Total messages: 8 (0 generated)
https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart File sdk/lib/io/stdio.dart (right): https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode51 sdk/lib/io/stdio.dart:51: bool retainNewlines: false}) { The retainNewlines parameter isn't documented. It seems to conflict with the "The line will contain the newline characters(s)" above. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode60 sdk/lib/io/stdio.dart:60: int b = readByteSync(); b -> byte https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode68 sdk/lib/io/stdio.dart:68: // We consider \r\n and \n as new lines. 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. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode69 sdk/lib/io/stdio.dart:69: // A \r on its own is treated like a normal character. treated like -> treated as https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode76 sdk/lib/io/stdio.dart:76: if (lastCharWasCR && !retainNewlines) { 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. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode96 sdk/lib/io/stdio.dart:96: if (lastCharWasCR) { && !retainNewLines ? https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcod... sdk/lib/io/stdio.dart:103: } 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.
lgtm, with Lasses comment addressed. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart File sdk/lib/io/stdio.dart (right): https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode48 sdk/lib/io/stdio.dart:48: * is returned. Please document the optional arguments.
My simple solution is too simple. Here is a more complete version (still not
tested).
bool crIsNewline = Platform.isWindows && !lineMode;
if (retainNewlines) {
int byte;
do {
byte = readByteSync();
if (byte < 0) break;
line.add(byte) ;
} while (byte != LF && !(byte == CR && crIsNewline));
if (line.isEmpty) return null;
} else if (crIsNewline) {
// CR and LF are both line terminators, neither is retained.
while (true);
int byte = readByteSync();
if (byte < 0) {
if (line.isEmpty) return null;
break;
}
if (byte == LF || byte == CR) break;
line.add(byte) ;
}
} else {
// Case having to hande CR LF as a single unretained line terminator.
outer: while (true) {
int byte = readByteSync();
if (byte == LF) break;
if (byte == CR) {
do {
byte = readByteSync();
if (byte == LF) break outer;
line.add(CR);
} while (byte == CR);
// Fall through and handle non-CR character.
}
if (byte < 0) {
if (line.isEmpty) return null;
break;
}
line.add(byte);
}
}
return encoding.encode(line);
PTAL https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart File sdk/lib/io/stdio.dart (right): https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode48 sdk/lib/io/stdio.dart:48: * is returned. On 2014/04/22 08:34:26, Søren Gjesse wrote: > Please document the optional arguments. Done. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode51 sdk/lib/io/stdio.dart:51: bool retainNewlines: false}) { On 2014/04/22 08:07:01, Lasse Reichstein Nielsen wrote: > The retainNewlines parameter isn't documented. > It seems to conflict with the "The line will contain the newline characters(s)" > above. Done. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode60 sdk/lib/io/stdio.dart:60: int b = readByteSync(); On 2014/04/22 08:07:01, Lasse Reichstein Nielsen wrote: > b -> byte Done. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode68 sdk/lib/io/stdio.dart:68: // We consider \r\n and \n as new lines. On 2014/04/22 08:07:01, Lasse Reichstein Nielsen wrote: > 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. Done. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode69 sdk/lib/io/stdio.dart:69: // A \r on its own is treated like a normal character. On 2014/04/22 08:07:01, Lasse Reichstein Nielsen wrote: > treated like -> treated as Done. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode76 sdk/lib/io/stdio.dart:76: if (lastCharWasCR && !retainNewlines) { On 2014/04/22 08:07:01, Lasse Reichstein Nielsen wrote: > 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. Done. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcode96 sdk/lib/io/stdio.dart:96: if (lastCharWasCR) { On 2014/04/22 08:07:01, Lasse Reichstein Nielsen wrote: > && !retainNewLines > ? Done. https://codereview.chromium.org/246973002/diff/1/sdk/lib/io/stdio.dart#newcod... sdk/lib/io/stdio.dart:103: } On 2014/04/22 08:07:01, Lasse Reichstein Nielsen wrote: > 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. Done.
lgtm https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart File sdk/lib/io/stdio.dart (right): https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart#ne... sdk/lib/io/stdio.dart:49: * final newline. Default is `false`. Doesn't say what happens if retainNewlines is true. I think it's best to be explicit, even if it's only, ", otherwise it will contain the line terminator." https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart#ne... sdk/lib/io/stdio.dart:51: * If end-of-file is reached after some data has already been read, that data How about "after any bytes have been read from stdin". I prefer "any" to "some", and it's not clear where it comes from or what the "data" is. "Returns `null` if no bytes preceeded the end of input." https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart#ne... sdk/lib/io/stdio.dart:88: outer: Indent label, and preferably put it on same line as the while.
Message was sent while issue was closed.
Committed patchset #3 manually as r35253 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart File sdk/lib/io/stdio.dart (right): https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart#ne... sdk/lib/io/stdio.dart:49: * final newline. Default is `false`. On 2014/04/22 10:56:08, Lasse Reichstein Nielsen wrote: > Doesn't say what happens if retainNewlines is true. I think it's best to be > explicit, even if it's only, ", otherwise it will contain the line terminator." Done. https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart#ne... sdk/lib/io/stdio.dart:51: * If end-of-file is reached after some data has already been read, that data On 2014/04/22 10:56:08, Lasse Reichstein Nielsen wrote: > How about "after any bytes have been read from stdin". I prefer "any" to "some", > and it's not clear where it comes from or what the "data" is. > "Returns `null` if no bytes preceeded the end of input." Done. https://codereview.chromium.org/246973002/diff/20001/sdk/lib/io/stdio.dart#ne... sdk/lib/io/stdio.dart:88: outer: On 2014/04/22 10:56:08, Lasse Reichstein Nielsen wrote: > Indent label, and preferably put it on same line as the while. Done. |
