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

fixed step_by change rust issue #27741 #27

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

william20111
Copy link
Contributor

Ignoring the rustfmt stuff, its just adding the step by feature as its changed. And it takes usize now so added a cast to that.

Rust - rust-lang/rust#27741

#![feature(iterator_step_by)]

@mehcode
Copy link
Contributor

mehcode commented Jul 6, 2017

I'd say we should just change the underlying types to usize rather than have the cast.

@william20111
Copy link
Contributor Author

Makes much more sense. Still getting to know the crate so might be better suited to somebody more comfortable with the code.

@zslayton
Copy link
Owner

zslayton commented Jul 7, 2017

@william20111 Thanks!

@mehcode It is my understanding that usize should be reserved for data that is dependent on the architecture (i.e. pointers/indexes). Some guidelines are discussed here. All of the values being represented in this crate have a very limited domain: 0-59 seconds/minutes, 0-23 hours, 0-31 days of the month, etc. I could safely represent them each as a single u8. In fact, I probably should since I'm storing several trees with dozens of values each and that would be a nice space savings.

I'm going to merge this PR. Please open an issue if you'd like to discuss it further.

@zslayton zslayton merged commit a4de18f into zslayton:master Jul 7, 2017
@mehcode
Copy link
Contributor

mehcode commented Jul 7, 2017

@zslayton Makes sense.

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.

3 participants