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

[breaking change] Stop exporting libc and cpp definitions. #86

Merged
merged 1 commit into from Dec 21, 2017

Conversation

@waywardmonkeys
Copy link
Collaborator

waywardmonkeys commented Dec 16, 2017

These definitions aren't needed and are just a relic of how bindgen
works. They shouldn't be exported and since they aren't used, they
don't need to exist.


This change is Reviewable

These definitions aren't needed and are just a relic of how bindgen
works. They shouldn't be exported and since they aren't used, they
don't need to exist.
@waywardmonkeys
Copy link
Collaborator Author

waywardmonkeys commented Dec 21, 2017

Ping? I would like to submit a PR that logically comes after this one so that there is no conflict.

@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 21, 2017

I'd like to get out of the habit of editing generated code (though I'm guilty of it myself), and instead use bindgen options to control the output when it is generated. Would you be interested in adding a bindgen script (it could be a simple shell script or similar), which we can then tweak to blacklist items that we don't want?

@waywardmonkeys
Copy link
Collaborator Author

waywardmonkeys commented Dec 21, 2017

The extra types involved are different on each platform which makes it a bit trickier.

I had been thinking of adding doc comments to this as well so that the Rust docs are more usable. (It ends up that Harfbuzz docs themselves are a bit scattered.) I have done this with some of my own crates where I don’t keep running bindgen. This also comes into play with enums and bitflags.

@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 21, 2017

Yeah, that makes sense. Any thoughts about how to handle regenerating bindings (e.g. for harfbuzz upgrades)? Maybe we should have a separate branch that just has the generated bindings, and merge that into the master branch (which includes patches to the bindings) whenever it changes.

@mbrubeck mbrubeck changed the title Stop exporting libc and cpp definitions. [breaking change] Stop exporting libc and cpp definitions. Dec 21, 2017
@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 21, 2017

Merging this for now; we can deal with the code generation issue later.

@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 21, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Dec 21, 2017

📌 Commit 823de4d has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Dec 21, 2017

Testing commit 823de4d with merge aec2173...

bors-servo added a commit that referenced this pull request Dec 21, 2017
[breaking change] Stop exporting libc and cpp definitions.

These definitions aren't needed and are just a relic of how bindgen
works. They shouldn't be exported and since they aren't used, they
don't need to exist.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/86)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 21, 2017

☀️ Test successful - status-travis
Approved by: mbrubeck
Pushing aec2173 to master...

@bors-servo bors-servo merged commit 823de4d into servo:master Dec 21, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@waywardmonkeys waywardmonkeys deleted the waywardmonkeys:remove-useless-exports branch Dec 22, 2017
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

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