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

Document why unwraps won't fail #63

Merged
merged 1 commit into from Jul 21, 2017

Conversation

Projects
None yet
4 participants
@shssoichiro
Copy link
Contributor

shssoichiro commented Jun 28, 2017

Closes #42

@KodrAus
Copy link
Contributor

KodrAus left a comment

Thanks @shssoichiro! This looks good to me.

@brson

This comment has been minimized.

Copy link

brson commented Jul 6, 2017

src/lib.rs Outdated
@@ -598,7 +601,8 @@ impl IntoIter {
// Make room for another open file descriptor if we've hit the max.
if self.stack_list.len() - self.oldest_opened == self.opts.max_open {
self.stack_list[self.oldest_opened].close();
self.oldest_opened = self.oldest_opened.checked_add(1).unwrap();
// Unwrap is safe here as long as we assume that `self.opts.max_open` is never 0

This comment has been minimized.

@BurntSushi

BurntSushi Jul 15, 2017

Owner

Hmm. I don't think this is right. While we do actually guarantee that self.ops.max_open is greater than 0, that doesn't seem to have anything to do with this unwrap. In particular, this unwrap is just making sure that we don't overflow the oldest_opened state, which is a usize. It's not technically a bug in walkdir if this trips, but it's more like an error case that really can't happen in practice. In particular, it would require descending into a directory that has a depth greater than the maximum size of a usize on the current platform.

Arguably, one might want to turn this into a real error, but it doesn't seem worth it to me. I think we should just add a comment that basically says what I said above and call it a day. What do you think?

This comment has been minimized.

@shssoichiro

shssoichiro Jul 16, 2017

Author Contributor

If walkdir guarantees that self.ops.max_open is never 0, then I can update this comment to indicate that. However, I wrote it this way because, given the arithmetic in the if clause, the checked_add will never fail if self.ops.max_open is greater than 0. Granted, it's still unrealistic that that bound would ever be reached. So perhaps, yes, I can update the comment to just indicate that.

This comment has been minimized.

@shssoichiro

shssoichiro Jul 16, 2017

Author Contributor

Upon further thought, is there a reason this was written as a checked_add rather than just using + 1?

This comment has been minimized.

@BurntSushi

BurntSushi Jul 17, 2017

Owner

Upon further thought, is there a reason this was written as a checked_add rather than just using + 1?

Because + 1 will silently overflow, which would be a silent bug.

But, you make a good point about the subtraction. Perhaps the right thing here is to change the if to self.stack_list.len().checked_sub(self.oldest_opened).unwrap() == self.ops.max_open and keep the checked_add. The comment can simply state that self.oldest_opened is guaranteed to never be greater than self.stack_list.len(), which implies that the subtraction won't underflow and that adding 1 will never overflow.

This comment has been minimized.

@shssoichiro

shssoichiro Jul 21, 2017

Author Contributor

That sounds good, I went ahead and pushed these changes up.

@BurntSushi BurntSushi merged commit d28c399 into BurntSushi:master Jul 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BurntSushi

This comment has been minimized.

Copy link
Owner

BurntSushi commented Jul 21, 2017

@shssoichiro Fantastic. Thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.