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

Remove the num-iter dependency. #150

Merged
merged 1 commit into from Jul 9, 2019
Merged

Remove the num-iter dependency. #150

merged 1 commit into from Jul 9, 2019

Conversation

RazrFalcon
Copy link
Contributor

@RazrFalcon RazrFalcon commented Jul 7, 2019

num-iter is used just for range_step iterator. By using std::iter::step_by we can remove a lot of bloat:

`-- num-iter v0.1.39
    |-- num-integer v0.1.41
    |   `-- num-traits v0.2.8
    |       [build-dependencies]
    |       `-- autocfg v0.1.4
    |   [build-dependencies]
    |   `-- autocfg v0.1.4 (*)
    `-- num-traits v0.2.8 (*)
    [build-dependencies]
    `-- autocfg v0.1.4 (*)

This also decreases the build time: 4.9s vs 6.7s

@HeroicKatora
Copy link
Member

Do we even need this anymore? (start..stop).step_by(step) seems to achive the very same thing from the standard library. Is that a performance difference?

@RazrFalcon
Copy link
Contributor Author

I've wanted to use step_by too, but it can step only by usize and this code uses isize.

@HeroicKatora
Copy link
Member

HeroicKatora commented Jul 7, 2019

bit_depth (the step argument) is a u8 initially, so that should be no problem. In fact, the line should have bit_depth.into() instead of bit_depth as isize to avoid making it seems like a lossy conversion.

@RazrFalcon
Copy link
Contributor Author

range_step used more than once.

@HeroicKatora
Copy link
Member

Oh, right 👍

@HeroicKatora
Copy link
Member

@RazrFalcon I'm kind of confused now. You said it is required to be used for isize as well but all usages cast to usize eventually. It seems all using functions expect the actual range of values to always be positive because they are used as indices (only within utils.rs). So, it should be possible to replace the isize in all iterators with carful application of .rev(), .step_by() and RangeInclusive (0..=buf.len()).

@HeroicKatora
Copy link
Member

E.g.

pub fn expand_trns_line(buf: &mut[u8], trns: &[u8], channels: usize) {
    let i = (0..=buf.len()/(channels+1)*channels - channels).rev().step_by(channels);
    let j = (0..=buf.len() - (channels+1)).rev().step_by(channels+1);
    let channels = channels as usize;
    for (i, j) in i.zip(j) {
        if &buf[i..i+channels] == trns {
            buf[j+channels] = 0
        } else {
            buf[j+channels] = 0xFF
        }
        for k in (0..channels).rev() {
            buf[j+k] = buf[i+k];
        }
    }
}

@RazrFalcon
Copy link
Contributor Author

Yes. I've simply replaced the current functions without investigating the code that much. I'm not familiar with the code, so I don't know why isize is used. I guess it simply steps from n to 0.

I will try to change it to step_by.

@RazrFalcon
Copy link
Contributor Author

Done.

@HeroicKatora HeroicKatora merged commit b8f576f into image-rs:master Jul 9, 2019
@HeroicKatora
Copy link
Member

HeroicKatora commented Jul 9, 2019

Great work, thank you! Want to join image-rs? You can be invited to the image-rs collaborators team, which allows you access to this and the other repos if you want. See the contribution policy and code of conduct. (I'll guess you have a ton of work from resvg already but asking won't hurt).

@RazrFalcon
Copy link
Contributor Author

Thanks, but I'm pretty busy with my own project already 😄

@HeroicKatora
Copy link
Member

If you ever change your mind, just ask (and probably link to this PR). Happy coding 😄

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

Successfully merging this pull request may close these issues.

None yet

2 participants