Skip to content

/ core-foundation-rs

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

Add bindings for `CFFileDescriptor` #84

Merged
merged 6 commits into from Jan 21, 2018
Merged

Add bindings for `CFFileDescriptor` #84

merged 6 commits into from Jan 21, 2018

Conversation

@trlim
Copy link
Contributor

trlim commented Sep 3, 2016 •
edited by larsbergstrom

This pull request adds implementation of CFFileDescriptor and CFRunLoopSource related API.

This change is Reviewable
@nox nox self-assigned this Sep 3, 2016
@nox
Copy link
Member

nox commented Sep 3, 2016

Pretty much every function in that PR that takes a raw pointer should be marked unsafe.
@trlim
Copy link
Contributor Author

trlim commented Sep 5, 2016 •
edited

I don't know if I'm getting right your point but I changed all(two) functions take raw pointer as unsafe.
But CFRunLoopTimer::new also should be marked unsafe in this sense.
@bors-servo
Copy link
Contributor

bors-servo commented Dec 21, 2016

☔ The latest upstream changes (presumably #87) made this pull request unmergeable. Please resolve the merge conflicts.
@nox nox removed their assignment Jul 4, 2017
Tae-il Lim added 4 commits Sep 3, 2016
Tae-il Lim
Implement CFFileDescriptor
530f732
Tae-il Lim
Safer handling of CFFileDescriptorContext
2a19e3f
Tae-il Lim
Add method to create CFRunLoopSource from CFFileDescriptor
c5d0134
Tae-il Lim
Add filedescriptor test
Loading status checks…
d6fe065
@trlim trlim force-pushed the trlim:filedescriptor branch from b4bb22d to d6fe065 Jan 15, 2018
@trlim
Copy link
Contributor Author

trlim commented Jan 15, 2018

I just updated this PR with rebased/reorganized commits and some changes.
The main difference is that CFFileDescriptor::new now has safe signature which takes optional reference to CFFileDescriptorContext and doesn't require unsafe pointer anymore.
@jdm
jdm requested changes Jan 17, 2018
View changes
Copy link
Member

jdm left a comment

These bindings look like a useful improvement. Thanks!
core-foundation/src/filedescriptor.rs Outdated
closeOnInvalidate: bool,
callout: CFFileDescriptorCallBack,
context: *const CFFileDescriptorContext) -> CFFileDescriptor {
let fd_ref = CFFileDescriptorCreate(kCFAllocatorDefault,

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

This can return null if there's a problem, according to the docs.
core-foundation/src/filedescriptor.rs Outdated
fd,
closeOnInvalidate as Boolean,
callout,
if let Some(context) = context {

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

Let's do this instead:

let context = context.map_or(ptr::null(), |c| c as *const _);
core-foundation/src/filedescriptor.rs Outdated
impl CFRunLoopSource {
pub fn from_file_descriptor(fd: &CFFileDescriptor, order: CFIndex) -> CFRunLoopSource {
unsafe {
let source_ref = CFFileDescriptorCreateRunLoopSource(kCFAllocatorDefault, fd.0, order);

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

This can return null according to the docs.
core-foundation/src/filedescriptor.rs Outdated
_callback_types: CFOptionFlags,
_info_ptr: *mut c_void) {
// should never be called
assert!(false);

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

Let's use unreachable!() instead.
core-foundation/src/filedescriptor.rs Outdated
let cf_fd = CFFileDescriptor::new(raw_fd, true, never_callback, None);

assert!(cf_fd.valid());
cf_fd.invalidate();

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

Let's assert !cf_fd.valid().
core-foundation/src/filedescriptor.rs Outdated
let cf_fd = CFFileDescriptor::new(raw_fd, false, never_callback, None);

assert!(cf_fd.valid());
cf_fd.invalidate();

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

Let's assert !cf_fd.valid().
core-foundation/src/filedescriptor.rs Outdated
}

extern "C" fn callback(_f: CFFileDescriptorRef, callback_types: CFOptionFlags, info_ptr: *mut c_void) {
assert!(info_ptr != ptr::null_mut());

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

assert!(!info_ptr.is_null());
core-foundation/src/filedescriptor.rs Outdated
assert_eq!(info.value, kCFFileDescriptorWriteCallBack);

info.value = 0;
cf_fd.disable_callbacks(kCFFileDescriptorReadCallBack|kCFFileDescriptorWriteCallBack);

This comment has been minimized.

Sign in to view
@jdm

jdm Jan 17, 2018

Member

nit: spaces around |
Tae-il Lim added 2 commits Jan 18, 2018
Tae-il Lim
Change create methods to return Option<T> to handle NULL return value
5285fe0
Tae-il Lim
Improve tests and code readability
Loading status checks…
9c36257
@trlim
Copy link
Contributor Author

trlim commented Jan 18, 2018

Applied changes as requested.
@jdm
Copy link
Member

jdm commented Jan 18, 2018

@bors-servo r+
Thanks!
@jdm
jdm approved these changes Jan 18, 2018
View changes
@jdm
Copy link
Member

jdm commented Jan 20, 2018

@bors-servo r+
@jdm jdm closed this Jan 20, 2018
@jdm jdm reopened this Jan 20, 2018
@jdm
Copy link
Member

jdm commented Jan 20, 2018

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 20, 2018

📌 Commit 9c36257 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 20, 2018

⌛ Testing commit 9c36257 with merge 2d35ed2...
bors-servo added a commit that referenced this pull request Jan 20, 2018
@bors-servo
Auto merge of #84 - trlim:filedescriptor, r=jdm
Loading status checks…
2d35ed2 
Add bindings for `CFFileDescriptor`

This pull request adds implementation of `CFFileDescriptor` and `CFRunLoopSource` related API.

<!-- Reviewable:start -->

---

This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/84)

<!-- Reviewable:end -->
@faern faern mentioned this pull request Jan 21, 2018
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2018

☀️ Test successful - status-travis
Approved by: jdm
Pushing 2d35ed2 to master...
@bors-servo bors-servo merged commit 9c36257 into servo:master Jan 21, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
jdm pushed a commit that referenced this pull request Feb 1, 2018
@bors-servo
Auto merge of #84 - servo:version, r=mbrubeck
558451d 
Update minor version.
jdm pushed a commit that referenced this pull request Feb 1, 2018
@bors-servo
Auto merge of #84 - poiru:typed-wrap-under-rule, r=jdm
80a8e93 
Use typed wrap_under_{create,get}_rule() functions

Just some minor cleanup; no functional changes.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-text-rs/84)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@trlim @nox @bors-servo @jdm
You can’t perform that action at this time.