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

Issue in reader #293

Open
wiluite opened this issue Dec 18, 2022 · 3 comments
Open

Issue in reader #293

wiluite opened this issue Dec 18, 2022 · 3 comments

Comments

@wiluite
Copy link

wiluite commented Dec 18, 2022

Possible error in code: file reader.rs

Check code if self.next_class = 255

existing code:
if self.next_class > CLASS_SIZE {

probably should be:
if self.next_class >= CLASS_SIZE {

const CLASS_SIZE: usize = 256;

    fn new() -> DfaClasses {
        DfaClasses {
            classes: [0; CLASS_SIZE],
            next_class: 1,
        }
    }

    fn add(&mut self, b: u8) {
        if self.next_class > CLASS_SIZE {
            panic!("added too many classes")
        }
        self.classes[b as usize] = self.next_class as u8;
        self.next_class += 1;
    }

May use controlled conversion to u8:

use std::convert::TryInto;
pub fn main() {
    // let mut c = DfaClasses::new();
    let b: u32 = 256;
    //let xxx: u8 = b as u8; // -> ignore overfow
    //let xxx: u8 = b.try_into().unwrap(); // -> runtime error: out of range in conversion
    let xxx: u8 = match b.try_into() {
        Result::Ok(b2) => b2,
        Result::Err(e2) => {
            println!("Error: {}", e2);
            0
        }
    }; // -> Error: out of range integral type conversion attempted
    // c.next_class = 255;
    // println!("{}", c.next_class);
    // c.add(123);
    // println!("{}", c.next_class);
    // c.add(123);
    println!("xxx = {}", xxx);
}
@BurntSushi
Copy link
Owner

Good catch. Patches welcome. Note that this error is impossible to trigger though, because the number of classes is always quite small and there is no way to make it bigger.

@wiluite
Copy link
Author

wiluite commented Jan 18, 2023

Yes, that's right.
But, nobody bothers to do this:
const CLASS_SIZE: usize = 3; ;-)

This issue was found by my colleage who just has no github account, so I'd written instead of him, and so I hardly supply a patch (Alas, don't know Rust). But. I have a side question: do you have benchmark statistics of this csv-reader on some of CSV-files (game.csv, for example). Could you give me a link?

@BurntSushi
Copy link
Owner

You can run benchmarks with cargo bench.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants