Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReaderUtil::each_byte includes trailing EOF #5056

Closed
evmar opened this issue Feb 20, 2013 · 9 comments
Closed

ReaderUtil::each_byte includes trailing EOF #5056

evmar opened this issue Feb 20, 2013 · 9 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@evmar
Copy link

evmar commented Feb 20, 2013

use io::ReaderUtil;

fn main() {
    let stdin = io::stdin();
    for stdin.each_byte() |b| {
        io::println(fmt!("byte '%c' %d", b as char, b));
    }
}
$ echo foo | ./main 
byte 'f' 102
byte 'o' 111
byte 'o' 111
byte '
' 10
byte '?' -1

That last byte shouldn't be included.

@evmar
Copy link
Author

evmar commented Feb 20, 2013

Using Rust 9143688 pulled yesterday.

@evmar
Copy link
Author

evmar commented Feb 20, 2013

Issue #2004 is probably related, in that it wasn't clear whether it's ok for read_byte to return -1.

@evmar
Copy link
Author

evmar commented Feb 22, 2013

Something like

From 7d649f37476f533de1bbd5c964350e7a9cc93ac3 Mon Sep 17 00:00:00 2001
From: Evan Martin <martine@danga.com>
Date: Wed, 20 Feb 2013 10:46:51 -0800
Subject: [PATCH] ReaderUtil::each_byte shouldn't include EOF byte -- Issue
 #5056

---
 src/libcore/io.rs | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/libcore/io.rs b/src/libcore/io.rs
index 2173efe..84877e7 100644
--- a/src/libcore/io.rs
+++ b/src/libcore/io.rs
@@ -277,12 +277,16 @@ impl<T: Reader> ReaderUtil for T {
     }

     fn each_byte(&self, it: fn(int) -> bool) {
-        while !self.eof() {
-            if !it(self.read_byte()) { break; }
+        loop {
+            match self.read_byte() {
+                -1 => break,
+                ch => if !it(ch) { break; }
+            }
         }
     }

     fn each_char(&self, it: fn(char) -> bool) {
+        // FIXME this doesn't handle EOF properly -- see issue #5056
         while !self.eof() {
             if !it(self.read_char()) { break; }
         }
@@ -1211,6 +1215,16 @@ mod tests {
     }

     #[test]
+    fn test_each_byte_empty() {
+        // Issue #5056 -- shouldn't include trailing EOF.
+        do io::with_str_reader(~"") |inp| {
+            for inp.each_byte() |b| {
+                assert(b != -1);
+            }
+        }
+    }
+
+    #[test]
     fn test_readchars_empty() {
         do io::with_str_reader(~"") |inp| {
             let res : ~[char] = inp.read_chars(128);
-- 
1.8.1.3

But the build is soooo slowwwwwww

@emberian
Copy link
Member

emberian commented Jul 1, 2013

@martine can you open a PR with that patch please?

@emberian
Copy link
Member

emberian commented Jul 1, 2013

(Reproduces)

@stepancheg
Copy link
Contributor

Pull request #8081.

@evmar
Copy link
Author

evmar commented Jul 27, 2013

Sorry for never cleaning up my initial patch, I think I had imagined things would be thrown away and redone before my patch was ever useful. I think ultimately it's pretty busted to provide a .eof() method because of the issue mentioned in your patch -- you can't know if you're at the end of a file until you attempt to read from it, so you need a way for read to signal eof, at which point you don't so much need a second API that tells you whether subsequent reads will fail.

@stepancheg
Copy link
Contributor

@martine I agree, .eof() is wrong operation for input stream. I'd drop it without replacement.

@stepancheg
Copy link
Contributor

Pull request merged into master, issue can be closed now.

bors added a commit to rust-lang-ci/rust that referenced this issue May 2, 2020
…1995

Avoid mut_key on types of unknown layout

This fixes rust-lang#5020 by requiring a known layout for the key type before linting. Edit: This fixes rust-lang#5043, too.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

4 participants