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

to_i32 shortcut for CFNumber #254

Merged
merged 1 commit into from Sep 24, 2018
Merged

to_i32 shortcut for CFNumber #254

merged 1 commit into from Sep 24, 2018

Conversation

@sibsibsib
Copy link
Contributor

sibsibsib commented Sep 13, 2018

kCFNumberIntType is needed in some iokit APIs.

Please let me know if you think anything is missing.


This change is Reviewable

pub fn to_u32(&self) -> Option<u32> {
unsafe {
let mut value: u32 = 0;
let ok = CFNumberGetValue(self.0, kCFNumberIntType, mem::transmute(&mut value));

This comment has been minimized.

@jdm

jdm Sep 14, 2018

Member

I think you want kCFNumberSInt32Type and cast the resulting i32 to u32. Otherwise I'm worried that the resulting number might be different sizes on 32 vs. 64 bit architectures.

This comment has been minimized.

@sibsibsib

sibsibsib Sep 14, 2018

Author Contributor

thanks for the feedback, I'll look into this 👍

@@ -27,7 +27,7 @@ pub static kCFNumberFloat32Type: CFNumberType = 5;
pub static kCFNumberFloat64Type: CFNumberType = 6;
// static kCFNumberCharType: CFNumberType = 7;
// static kCFNumberShortType: CFNumberType = 8;
// static kCFNumberIntType: CFNumberType = 9;
pub static kCFNumberIntType: CFNumberType = 9;

This comment has been minimized.

@jdm

jdm Sep 14, 2018

Member

This change can be reverted with my suggestion.

unsafe {
let number_ref = CFNumberCreate(
kCFAllocatorDefault,
kCFNumberIntType,

This comment has been minimized.

@jdm

jdm Sep 14, 2018

Member

kCFNumberSInt32Type

@jrmuizel
Copy link
Collaborator

jrmuizel commented Sep 14, 2018

CoreFoundation doesn't support a u32 type. Why do you need to add it?

@sibsibsib sibsibsib force-pushed the sibsibsib:master branch from 904fe46 to 6b3205e Sep 23, 2018
@sibsibsib sibsibsib changed the title enable u32 for CFNumber to_i32 shortcut for CFNumber Sep 23, 2018
@sibsibsib
Copy link
Contributor Author

sibsibsib commented Sep 23, 2018

I did some digging and found I am able to use the SInt32Type for the APIs I'm working with. As such, I've removed the u32-related code and left the to_i32 shortcut. Thanks for the feedback!

@jdm
Copy link
Member

jdm commented Sep 24, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2018

📌 Commit 6b3205e has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2018

Testing commit 6b3205e with merge febdc72...

bors-servo added a commit that referenced this pull request Sep 24, 2018
to_i32 shortcut for CFNumber

kCFNumberIntType is needed in some iokit APIs.

Please let me know if you think anything is missing.

<!-- 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/254)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2018

☀️ Test successful - status-travis
Approved by: jdm
Pushing febdc72 to master...

@bors-servo bors-servo merged commit 6b3205e into servo:master Sep 24, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.