Skip to content

Conversation

@fiveop
Copy link
Contributor

@fiveop fiveop commented May 4, 2016

This fixes an warning made error on nightly builds.

@fiveop
Copy link
Contributor Author

fiveop commented May 4, 2016

r? @kamalmarhubi

src/unistd.rs Outdated
Errno::result(res).map(drop)
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for adding these 2 lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I'll fix it.

@fiveop
Copy link
Contributor Author

fiveop commented May 6, 2016

I readded the (reworded) comment.

src/unistd.rs Outdated
use std::num::Wrapping;
unsafe { libc::chown(cstr.as_ptr(),
owner.unwrap_or((Wrapping(0 as uid_t) - Wrapping(1 as uid_t)).0),
group.unwrap_or((Wrapping(0 as gid_t) - Wrapping(1 as gid_t)).0)) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last nit, can we use 0.wrapping_sub(1) instead of using Wrapping? I think it'll be easier to read and doesn't require an extra import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly! If I had known the function, I would have used it right away.

@kamalmarhubi
Copy link
Member

@homu r+ 2139c90

homu added a commit that referenced this pull request May 6, 2016
Use Wrapping for intended underflow of unsigned integer value.

This fixes an warning made error on nightly builds.
@homu
Copy link
Contributor

homu commented May 6, 2016

⌛ Testing commit 2139c90 with merge 7931e48...

@homu
Copy link
Contributor

homu commented May 6, 2016

☀️ Test successful - status

@homu homu merged commit 2139c90 into nix-rust:master May 6, 2016
@fiveop fiveop deleted the fixoverflow branch May 7, 2016 12:21
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.

4 participants