Skip to content
This repository was archived by the owner on Jul 10, 2023. It is now read-only.

Fix core-text related warnings (#62) #65

Merged
merged 1 commit into from
Jul 5, 2017
Merged

Fix core-text related warnings (#62) #65

merged 1 commit into from
Jul 5, 2017

Conversation

omakk
Copy link
Contributor

@omakk omakk commented Jul 4, 2017

Fix private-in-public warnings (error E0446)

Fixed by prepending pub to the offending types. Not sure if its the best way but it was the only thing that worked. Other methods I've tried to maintain privateness:

  • Replace with an empty enum. That threw an error.
  • Make it public and wrap within a private module. That threw an error too as #[repr(C)] does not accept modules

Fix use of extern static warnings by wrapping in unsafe block (error E0133)

I think this is self-explanatory.

Fix zero-size struct warnings on certain CT* types

Wrap around c_void as I believe the types are only used as pointers. Again, let me know if there is a better way to do this. I've tried following this discussion on opaque C types but I don't think there is a consensus. I came across this discussion from the Rust SDL2 bindings and they eventually opted for the solution that I implemented.

This is my first pull request (hence, first open source contribution) and I've been playing with Rust for the past few weeks. So please, if anything isnt up to standards or correctness, let me know! Also, to squash the other warnings, those changes would have to made in the modules where those types reside (core-graphics and core-foundation). I would gladly open a PR to fix those too if all looks good.

Thanks!


This change is Reviewable

Fix private-in-public warnings (error E0446)

Fix use of extern static warnings by wrapping in unsafe block (error E0133)

Fix zero-size struct warnings on certain CT* types
@jdm
Copy link
Member

jdm commented Jul 5, 2017

@bors-servo: r+
Looks good! Thank you for dealing with this!

@bors-servo
Copy link

📌 Commit 6847e69 has been approved by jdm

@bors-servo
Copy link

⌛ Testing commit 6847e69 with merge a2368a0...

bors-servo pushed a commit that referenced this pull request Jul 5, 2017
Fix core-text related warnings (#62)

### Fix private-in-public warnings (error E0446)
Fixed by prepending `pub` to the offending types. Not sure if its the best way but it was the only thing that worked. Other methods I've tried to maintain privateness:
* Replace with an empty enum. That threw an error.
* Make it public and wrap within a private module. That threw an error too as `#[repr(C)]` does not accept modules

### Fix use of extern static warnings by wrapping in unsafe block (error E0133)
I think this is self-explanatory.

### Fix zero-size struct warnings on certain CT* types
Wrap around c_void as I believe the types are only used as pointers. Again, let me know if there is a better way to do this. I've tried following [this discussion](rust-lang/rust#27303) on opaque C types but I don't think there is a consensus. I came across this [discussion](Rust-SDL2/rust-sdl2#442) from the Rust SDL2 bindings and they eventually [opted](Rust-SDL2/rust-sdl2#445) for the solution that I implemented.

This is my first pull request (hence, first open source contribution) and I've been playing with Rust for the past few weeks. So please, if anything isnt up to standards or correctness, let me know! Also, to squash the other warnings, those changes would have to made in the modules where those types reside (core-graphics and core-foundation). I would gladly open a PR to fix those too if all looks good.

Thanks!

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

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

@bors-servo bors-servo merged commit 6847e69 into servo:master Jul 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants