Feature 832 custom rust target by tmfink · Pull Request #859 · rust-lang/rust-bindgen

tmfink · 2017-07-27T16:41:16Z

Addresses #832.

Instead of specifying whether or not to use stable, specify the Rust
release to support (one of several stable/beta releases or nightly).
The --unstable-rust option is still accepted and implies --rust-target nightly.

The definitions of RustTarget and RustFeatures are created with
macros.

In order to keep the test outputs the same,
bindgen-flags: --rust-target 1.0 was added to test headers.

Todo:

Create RustFeatures/RustTarget structs
Replace uses of unstable with RustFeatures query
Add new tests
Fix doc comments TODOs

tmfink · 2017-07-30T20:59:41Z

r? @fitzgen or @emilio

bors-servo · 2017-07-31T20:03:08Z

☔ The latest upstream changes (presumably #861) made this pull request unmergeable. Please resolve the merge conflicts.

fitzgen · 2017-08-01T15:51:33Z

I meant to review this yesterday, but didn't end up with enough time. I'll do my best to get to it today!

Thanks for the patience @tmfink :)

bors-servo · 2017-08-01T16:56:40Z

☔ The latest upstream changes (presumably #874) made this pull request unmergeable. Please resolve the merge conflicts.

fitzgen

This turned out really nice, and was way less invasive than I feared it might have been :)

I have a couple nitpicks below, and then we can merge this :)

Thanks! And sorry for the delay!

fitzgen · 2017-08-03T17:30:47Z

src/features.rs

+/// Define RustTarget struct definition, Default impl, and conversions
+/// between RustTarget and String.
+macro_rules! rust_target_def {
+    ( $( $release:ident => $value:expr => $attrs:meta; )* ) => {


This would be more natural with the attrs in the front:

( $( $(#[$attr:meta])* $release:ident => $value:expr ; )* ) => { ... }

And then we could use it like this:

rust_target_def!( /// Rust stable 1.0 Stable_1_0 => 1.0; /// Rust stable 1.19 Stable_1_19 => 1.19; /// Nightly rust Nightly => nightly; );

Wow, I didn't realize you could do that.

fitzgen · 2017-08-03T17:38:29Z

src/features.rs

+            /// * The stable/beta versions of Rust are of the form "1.0",
+            /// "1.19", etc.
+            /// * The nightly version should be specified with "nightly".
+            pub fn from_str<'a>(s: &'a str) -> io::Result<RustTarget> {


This should be impl FromStr for RustTarget: https://doc.rust-lang.org/nightly/std/str/trait.FromStr.html

Nice, I did not know that existed.

fitzgen · 2017-08-03T17:40:16Z

src/features.rs

+
+rust_feature_def!(
+    untagged_union => doc="Untagged unions ([RFC 1444](https://github.com/rust-lang/rfcs/blob/master/text/1444-union.md))";
+    const_fn => doc="Constant function ([RFC 911](https://github.com/rust-lang/rfcs/blob/master/text/0911-const-fn.md))";


Similar to the above comment: this would read a lot more naturally if the macro was refactored so that we could use normal doc comment sugar.

fitzgen · 2017-08-03T17:50:16Z

src/lib.rs

-    pub fn unstable_rust(mut self, doit: bool) -> Self {
-        self.options.unstable_rust = doit;
-        self
+    #[deprecated(note="please use `rust_target` instead")]


Thanks for this detail :)

fitzgen · 2017-08-03T17:55:34Z

src/options.rs

                .number_of_values(1),
+            Arg::with_name("rust-target")
+                .long("rust-target")
+                .help("Version of the Rust compiler to target.")


This should list either

The exact set of supported target versions (as the string one would give to this flag). This would involve getting a string out of the macro somehow.

or we should define a RustTarget for every stable version of rust, and have an example of targeting a stable version and an example of targeting nightly here.

What we shouldn't do is leave users to guess how to format the string: is it "1.19" or "stable-1.19" or "stable" or "stable 1.19" or ...?? And if "1.19" works, then why doesn't "1.18" work?

I decided to use the macros.

fitzgen · 2017-08-03T18:14:06Z

One more thing: can you rebase and squash these commits into a single commit on top of master? Thanks!

bors-servo · 2017-08-04T01:19:11Z

☔ The latest upstream changes (presumably #866) made this pull request unmergeable. Please resolve the merge conflicts.

tmfink · 2017-08-04T09:53:11Z

Looks like there were some issues with my rebase--I'll fix it tomorrow.

fitzgen · 2017-08-04T16:32:17Z

Looks like there were some issues with my rebase--I'll fix it tomorrow.

Ok! Let me know if you have any questions or need any help or anythign :)

Instead of specifying whether or not to use stable, specify the Rust release to support (one of several stable/beta releases or nightly). The --unstable-rust option is still accepted and implies nightly. The definitions of `RustTarget` and `RustFeatures` are created with macros. For each test that uses unions, there is a version that uses the latest stable release and stable 1.0.

tmfink · 2017-08-04T16:54:34Z

I squashed everything, but the downside is that it lost "git copy" trick that I used to keep the provenance of the "1.0" tests. Now Git thinks that I made the headers/expectations from scratch.

fitzgen · 2017-08-04T17:03:02Z

While that's too bad, ultimately, I don't think it's a huge deal.

fitzgen · 2017-08-04T17:16:55Z

src/lib.rs

    include!(concat!(env!("OUT_DIR"), "/codegen.rs"));
 }

+pub use features::{RustTarget, LATEST_STABLE_RUST, RUST_TARGET_STRINGS};


I don't see RUST_TARGET_STRINGS used in this module, is it a leftover from some earlier iteration?

Huh, well there are no warnings in the travis ci compilation (and I guess we deny(warnings) anyhow).

fitzgen · 2017-08-04T17:20:33Z

@bors-servo r+

Thanks @tmfink !

bors-servo · 2017-08-04T17:20:33Z

📌 Commit 0bb7b9f has been approved by fitzgen

bors-servo · 2017-08-04T17:20:36Z

⌛ Testing commit 0bb7b9f with merge de180c4...

Feature 832 custom rust target Addresses #832. Instead of specifying whether or not to use stable, specify the Rust release to support (one of several stable/beta releases or nightly). The `--unstable-rust` option is still accepted and implies `--rust-target nightly`. The definitions of `RustTarget` and `RustFeatures` are created with macros. In order to keep the test outputs the same, `bindgen-flags: --rust-target 1.0` was added to test headers. **Todo:** - [x] Create `RustFeatures`/`RustTarget` structs - [x] Replace uses of `unstable` with `RustFeatures` query - [x] Add new tests - [x] Fix doc comments TODOs

bors-servo · 2017-08-04T18:02:11Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing de180c4 to master...

tmfink mentioned this pull request Aug 1, 2017

Complete bindings with Bindgen capstone-rust/capstone-sys#1

Merged

6 tasks

fitzgen reviewed Aug 3, 2017

View reviewed changes

tmfink force-pushed the feature-832-custom-rust-target branch from ec70f89 to 476d1ea Compare August 4, 2017 09:47

tmfink force-pushed the feature-832-custom-rust-target branch from 476d1ea to 0bb7b9f Compare August 4, 2017 16:52

fitzgen approved these changes Aug 4, 2017

View reviewed changes

bors-servo merged commit 0bb7b9f into rust-lang:master Aug 4, 2017

bors-servo mentioned this pull request Aug 4, 2017

Can derive hash analysis #887

Merged

fitzgen mentioned this pull request Aug 4, 2017

Start emitting unions in stable mode #832

Closed

Conversation

tmfink commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmfink commented Jul 30, 2017

Uh oh!

bors-servo commented Jul 31, 2017

Uh oh!

fitzgen commented Aug 1, 2017

Uh oh!

bors-servo commented Aug 1, 2017

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzgen commented Aug 3, 2017

Uh oh!

bors-servo commented Aug 4, 2017

Uh oh!

tmfink commented Aug 4, 2017

Uh oh!

fitzgen commented Aug 4, 2017

Uh oh!

tmfink commented Aug 4, 2017

Uh oh!

fitzgen commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzgen commented Aug 4, 2017

Uh oh!

bors-servo commented Aug 4, 2017

Uh oh!

bors-servo commented Aug 4, 2017

Uh oh!

bors-servo commented Aug 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tmfink commented Jul 27, 2017 •

edited

Loading