11
\$\begingroup\$

I'm learning Rust, from a Python background, and while I've used languages like C and C++ in the (distant) past, system languages aren't really my specialty.

I would just like to know if my code is sane, if I seem to have the principles roughly right, but I'd be happy to hear any improvements at all, particularly with performance. I'm also a bit unsure about error handling.

Anyway, for my first program I've made a simple implementation of the UNIX cat command, without support for any arguments:

use std::env;
use std::io;
use std::io::Read;
use std::io::Write;
use std::fs::File;


macro_rules! println_stderr(
    ($($arg:tt)*) => (
        match writeln!(&mut io::stderr(), $($arg)* ) {
            Ok(_) => {},
            Err(err) => panic!("Unable to write to stderr: {}", err),
        }
    )
);


fn main() {
    let args: Vec<_> = env::args().collect();
    if args.len() < 2 {
        loop {
            let mut s = String::new();
            match io::stdin().read_line(&mut s) {
                Ok(_) => {},
                Err(err) => {
                    println_stderr!("{}", err.to_string());
                    continue;
                }
            };
            print!("{}", s);
        }
    } else {
        for arg in &args[1..] {
            let mut s = String::new();
            let mut file = match File::open(arg) {
                Ok(file) => file,
                Err(err) => {
                    println_stderr!("{}", err.to_string());
                    continue;
                }
            };

            match file.read_to_string(&mut s) {
                Ok(_) => {},
                Err(err) => {
                    println_stderr!("{}", err.to_string());
                    continue;
                }
            };

            print!("{}", s);
        }
    }
}
\$\endgroup\$
13
\$\begingroup\$

imports

Your imports should be compressed:

use std::io::{self, Read, Write};

Although it might be better to use the io prelude:

use std::io;
use std::io::prelude::*;

I prefer the former for its explicitness.

match vs if let

match io::stdin().read_line(&mut s) {
    Ok(_) => {},
    Err(err) => {
        println_stderr!("{}", err.to_string());
        continue;
    }
};

is better as just

if let Err(err) = io::stdin().read_line(&mut s) {
    println_stderr!("{}", err);
    continue;
};

The same occurs later, and in the macro.

prefer late initialization

You should only initialize s just before use in the second branch.

copy-paste error?

Your continue when reading from stdin is a bit optimistic. I'd suggest just exiting at that point - if reading one line fails then reading the rest probably will too.

plz no Unicode

You read and write strings holding the whole file (or line) - cat should work instead on raw bytes, and preferrably in large chunks too. The most obvious way is something like

fn redirect_stream<R, W>(reader: &mut R, writer: &mut W, buffer: &mut Vec<u8>) -> io::Result<()>
    where R: Read, W: Write
{
    let mut buffer = vec![0; 64 * 1024];

    loop {
        let len_read = try!(reader.read(&mut buffer));

        if len_read == 0 {
            return Ok(())
        }

        try!(writer.write_all(&buffer[..len_read]));
    }
}

plz no allocate

Using the above means we're stuck allocating 64k for each stream, even if there are many streams or the stream is line-buffered.

The first can be solved by passing the buffer into the function. The later can be solved by resizing the buffer up to some hard limit from a small size.

DRY

Note that there's still duplication between the args.len() < 2 and else branches. One could solve this by writing a wrapping function to extract this functionality.

fn handle_arg<R, W>(reader: &mut R, writer: &mut W, buffer: &mut Vec<u8>)
    where R: Read, W: Write
{
    if let Err(err) = redirect_stream(reader, writer, buffer) {
        println_stderr!("{}", err.to_string());
    }
}

Making handle_arg a closure would be prettier but require dynamic dispatch. That's probably fine, but maybe less idiomatic:

let stdout = &mut io::stdout();
let buffer = &mut vec![0; SMALL_BUFFER_SIZE];
let mut handle_arg = move |mut reader: &mut Read| {
    if let Err(err) = redirect_stream(&mut reader, stdout, buffer) {
        println_stderr!("{}", err.to_string());
    }
};

- means stdin?

This is more reasonable if we want to allow the code to accept - to mean standard input, as we can do

let mut args: Vec<_> = env::args().skip(1).collect();
if args.is_empty() {
    args.push("-".into());
}

let stdout = &mut io::stdout();
let buffer = &mut vec![0; SMALL_BUFFER_SIZE];
for arg in args {
    if arg == "-" {
        handle_arg(&mut io::stdin(), stdout, buffer);
        continue;
    }

    match File::open(arg) {
        Ok(ref mut file) => {
            handle_arg(file, stdout, buffer)
        },
        Err(err) => {
            println_stderr!("{}", err);
            continue;
        }
    }
}

Result

use std::env;
use std::io::{self, Read, Write};
use std::iter;
use std::fs::File;

const SMALL_BUFFER_SIZE: usize = 256;
const LARGE_BUFFER_SIZE: usize = 64 * 1024;


macro_rules! println_stderr(
    ($($arg:tt)*) => (
        if let Err(err) = writeln!(&mut io::stderr(), $($arg)* ) {
            panic!("Unable to write to stderr: {}", err);
        }
    )
);


fn redirect_stream<R, W>(reader: &mut R, writer: &mut W, buffer: &mut Vec<u8>) -> io::Result<()>
    where R: Read, W: Write
{
    loop {
        let len_read = try!(reader.read(buffer));

        if len_read == 0 {
            return Ok(())
        }

        try!(writer.write_all(&buffer[..len_read]));

        if len_read == buffer.len() && len_read < LARGE_BUFFER_SIZE {
            buffer.extend(iter::repeat(0).take(len_read));
        }
    }
}


fn main() {
    let mut args: Vec<_> = env::args().skip(1).collect();
    if args.is_empty() {
        args.push("-".into());
    }

    fn handle_arg<R, W>(reader: &mut R, writer: &mut W, buffer: &mut Vec<u8>)
        where R: Read, W: Write
    {
        if let Err(err) = redirect_stream(reader, writer, buffer) {
            println_stderr!("{}", err.to_string());
        }
    }

    let stdout = &mut io::stdout();
    let buffer = &mut vec![0; SMALL_BUFFER_SIZE];
    for arg in args {
        if arg == "-" {
            handle_arg(&mut io::stdin(), stdout, buffer);
            continue;
        }

        match File::open(arg) {
            Ok(ref mut file) => {
                handle_arg(file, stdout, buffer)
            },
            Err(err) => {
                println_stderr!("{}", err);
                continue;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Wow, thanks! I didn't realise my 50 line stab in the dark could be improved so much. I'll come back later when I'm feeling more rusty with more questions, but could you explain use std::io::prelude::*;? Is it just a commonly used subset of std::io? I follow the Zen of Python in that explicit is better than implicit, so my preference will probably align with yours there. \$\endgroup\$ – Tom Carrick Jun 29 '15 at 14:53
  • \$\begingroup\$ "Is it just a commonly used subset of std::io?" → Yep (docs). Mostly it's to import all the traits you'd expect things to have, since Rust is unusual in that each interface must be imported separately. I'm not a fan, but I suppose it exists for a reason ;). \$\endgroup\$ – Veedrac Jun 29 '15 at 15:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.