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

Fix c_str redesign errors #10

Merged
merged 8 commits into from
Jan 12, 2015
Merged

Fix c_str redesign errors #10

merged 8 commits into from
Jan 12, 2015

Conversation

dead10ck
Copy link
Contributor

@dead10ck dead10ck commented Jan 9, 2015

@hannobraun
Copy link
Owner

Aside from the offset(1) thing, which I'm unsure about, all looks well! Thank you, @dead10ck!

The offset seems to be used for the name of the file that's changed. None of the automated tests cover this functionality. I think the best solution would be to add a test for this, so we can be sure the code was right. As before, I will get to it on Monday, or you can take a look, if you want.

Thanks again for all your work!

@hannobraun
Copy link
Owner

Regarding the unstable warnings, I've opened a separate issue: #11.

@dead10ck
Copy link
Contributor Author

@hannobraun yeah, the offset thing should be checked before this is merged, but I won't have time to do it this weekend :/ and no problem!

@hannobraun
Copy link
Owner

@dead10ck Ok, I'll pick it up on Monday then :)

@hannobraun hannobraun merged commit 1f36802 into hannobraun:master Jan 12, 2015
@hannobraun
Copy link
Owner

Merged manually. The offset(1) was indeed needed. The reason is that the inotify_event struct originally contains an optional string as its last field. Since I don't know how to represent that in Rust (not sure it can be), this requires some trickery when reading the name. See the man page for all the details.

Thanks again, @dead10ck, for the great work!

@dead10ck
Copy link
Contributor Author

Ahh, I see. Sorry, I'm still new to Rust, and I haven't worked with lower-level FFI stuff yet, so I wasn't familiar with how all of that worked. I'm glad I was able to help with the other stuff though :)

@hannobraun
Copy link
Owner

No need to apologize, that part of the code is a bit icky. We have a test for it now, so this problem shouldn't come up any more.

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.

2 participants