Clap your hands say yeah! 👏🏻 #228

jdub · 2016-11-08T09:25:04Z

In progress port to clap, obsoletes the docopt port in #202.

jdub · 2016-11-08T09:27:52Z

One thing that's weird about this is that --output file name is parsed from the parameters, and most of Builder is private, so it can't be passed to Bindings::write(out: Box<io::Write>).

Maybe just replace write(Box<io::Write>) with write_to_output()?

jdub · 2016-11-08T11:15:34Z

Some notes:

I don't really want all of this stuck in libbindgen, but in bindgen and the libbindgen test harness, but it was easier to get started this way (maybe it should be a shared module… probably not a whole crate)
Instead of all that if damage, I should see if I can loop around the contents of matches

emilio · 2016-11-08T13:34:27Z

I don't really want all of this stuck in libbindgen, but in bindgen and the libbindgen test harness, but it was easier to get started this way (maybe it should be a shared module… probably not a whole crate)

Probably a module annotated with #[path=""] is not too hard?

emilio

Looks awesome! Thanks for doing this!

emilio · 2016-11-08T13:37:55Z

src/lib.rs

+    // clap doesn't support the end of options marker, --
+    // https://github.com/kbknapp/clap-rs/issues/735
+    let mut terminated = false;
+    let (bindgen_args, clang_args): (Vec<_>, Vec<_>) = args.partition(|arg| {


Probably if arg was a slice, it'd be easier to do something like postition(), and then split_at.

emilio · 2016-11-08T13:39:24Z

src/lib.rs

+    // https://github.com/kbknapp/clap-rs/issues/735
+    let mut terminated = false;
+    let (bindgen_args, clang_args): (Vec<_>, Vec<_>) = args.partition(|arg| {
+        if terminated { // clang arg


can we simplify this as:

if arg == "--" { terminated = true; } !terminated

Probably renaming terminated to in_clang_args or something like that would also be neat.

emilio · 2016-11-08T13:41:37Z

src/lib.rs

+
+    let mut builder = builder();
+
+    if let Some(header) = matches.value_of("header") {


instead of if let Some(..) { } else { }, what do you think about:

let builder = match matches.value_of("header") { Some(h) => builder.header(h), None => return Err(..), };

I don't feel too strongly about it, so feel free to leave as-is if you prefer.

I feel weird about putting an early return in a match arm… going to leave this for now, but it's ultimately your call!

emilio · 2016-11-08T13:43:43Z

src/lib.rs

+    }
+
+    // FIXME: hrrrmmm
+    let _output = if let Some(path) = matches.value_of("output") {


Since this is a helper, I guess this can return Result<Builder, Option<Box<io::Write>> (I'd rather decide what to do with the missing output case in the caller).

Whoops, I meant Result<(Builder, Option<Box<io::Write>), io::Error>

I've done this, though realised that the output is not optional the way it works now -- it returns an io::Write for a file or stdout. So that'll make it slightly simpler.

emilio · 2016-11-08T13:44:38Z

src/bin/bindgen.rs

@@ -294,14 +66,20 @@ pub fn main() {
        }
    }

-    let (options, out) = parse_args_or_exit(bind_args);
+    if let Ok(builder) = bindgen::builder_from_flags(env::args()) {
+        //println!("{:#?}", builder);


This needs to go away before merging of course.

emilio · 2016-11-08T13:46:52Z

src/bin/bindgen.rs

@@ -294,14 +66,20 @@ pub fn main() {
        }
    }

-    let (options, out) = parse_args_or_exit(bind_args);


This would remain something like:

let (options, out) = match bindgen::builder_from_flags(env::args()) { Ok((builder, Some(out)) => (builder, out), _ => // Exit, I guess. };

jdub · 2016-11-09T11:48:53Z

Considering #204, can I put the command line parsing stuff in bindgen and use it with #[path=""] from the libbindgen test suite? Would files from bindgen even be present in a libbindgen crate?

emilio · 2016-11-09T12:03:32Z

Probably as long as we keep it in the same repo it's fine. We can also include! it at build time, I guess. We can figure that out when needed.

emilio · 2016-11-09T12:04:05Z

Please ping when it's ready for review! :)

jdub · 2016-11-09T12:45:29Z

Alrighty, it's review time! 😄

emilio

Whohoo, looks great! I need to double check every flag is in the list, but with that and those two comments, this should be ready to go IMO.

Thanks for doing this 🎉

emilio · 2016-11-09T23:09:17Z

src/lib.rs

@@ -85,7 +85,7 @@ use regex_set::RegexSet;

 use std::borrow::Borrow;
 use std::collections::HashSet;
-use std::fs::OpenOptions;
+use std::fs::{OpenOptions};


nit: Revert?

emilio · 2016-11-09T23:10:04Z

src/bin/bindgen.rs

+            bindings.write_dummy_uses()
+                .expect("Unable to write dummy uses to file.");
+        }
+        Err(error) => println!("{}", error)


nit: We most likely want to exit with a nonzero error code here. Panicking may be ok, other kind of handling even better :)

jdub · 2016-11-10T01:45:41Z

Nits fixed.

emilio · 2016-11-10T08:56:38Z

@bors-servo r+

Thanks! :)

emilio · 2016-11-10T09:00:02Z

Trying to make bors hear me.

emilio · 2016-11-10T09:00:12Z

@bors-servo r+

bors-servo · 2016-11-10T09:00:14Z

📌 Commit dc96c1f has been approved by emilio

bors-servo · 2016-11-10T09:00:18Z

⌛ Testing commit dc96c1f with merge b3322dd...

Clap your hands say yeah! 👏🏻 In progress port to `clap`, obsoletes the `docopt` port in #202.

bors-servo · 2016-11-10T09:36:15Z

☀️ Test successful - status-travis

@fitzgen

r? @fitzgen Fixes #242

@fitzgen

Fix path detection after #228 r? @fitzgen Too bad we can't test it on CI because CI has the path correctly setup :( Fixes #242

highfive added the S-awaiting-review label Nov 8, 2016

jdub mentioned this pull request Nov 8, 2016

Split off bindgen library into a sub-crate #204

Merged

emilio reviewed Nov 8, 2016

View reviewed changes

jdub force-pushed the clap branch 2 times, most recently from aadd44f to 11f91c8 Compare November 9, 2016 05:47

Add required Builder methods

6e2397f

jdub force-pushed the clap branch from 3c22c3c to f78a573 Compare November 9, 2016 12:45

jdub force-pushed the clap branch from f78a573 to a76438a Compare November 9, 2016 12:47

emilio approved these changes Nov 9, 2016

View reviewed changes

Port to clap command line parsing

dc96c1f

jdub force-pushed the clap branch from a76438a to dc96c1f Compare November 10, 2016 01:45

highfive assigned emilio Nov 10, 2016

emilio closed this Nov 10, 2016

emilio reopened this Nov 10, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 10, 2016

bors-servo pushed a commit that referenced this pull request Nov 10, 2016

Auto merge of #228 - jdub:clap, r=emilio

b3322dd

Clap your hands say yeah! 👏🏻 In progress port to `clap`, obsoletes the `docopt` port in #202.

bors-servo merged commit dc96c1f into rust-lang:master Nov 10, 2016

highfive removed the S-awaiting-merge label Nov 10, 2016

jdub deleted the clap branch November 10, 2016 11:27

emilio mentioned this pull request Nov 13, 2016

Can't find system headers #242

Closed

bors-servo pushed a commit that referenced this pull request Nov 14, 2016

Fix path detection after #228

4640eca

r? @fitzgen Fixes #242

bors-servo pushed a commit that referenced this pull request Nov 14, 2016

Auto merge of #243 - emilio:include-path, r=fitzgen

b2081eb

Fix path detection after #228 r? @fitzgen Too bad we can't test it on CI because CI has the path correctly setup :( Fixes #242

jdub mentioned this pull request Nov 16, 2016

Re-introduce docopt? #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clap your hands say yeah! 👏🏻 #228

Clap your hands say yeah! 👏🏻 #228

jdub commented Nov 8, 2016

jdub commented Nov 8, 2016 •

edited

Loading

jdub commented Nov 8, 2016

emilio commented Nov 8, 2016

emilio left a comment

emilio Nov 8, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

jdub Nov 9, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

jdub Nov 9, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

jdub commented Nov 9, 2016

emilio commented Nov 9, 2016

emilio commented Nov 9, 2016

jdub commented Nov 9, 2016

emilio left a comment

emilio Nov 9, 2016

emilio Nov 9, 2016

jdub commented Nov 10, 2016

emilio commented Nov 10, 2016 •

edited

Loading

emilio commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016


		let mut builder = builder();

		if let Some(header) = matches.value_of("header") {

Clap your hands say yeah! 👏🏻 #228

Clap your hands say yeah! 👏🏻 #228

Conversation

jdub commented Nov 8, 2016

jdub commented Nov 8, 2016 • edited Loading

jdub commented Nov 8, 2016

emilio commented Nov 8, 2016

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdub commented Nov 9, 2016

emilio commented Nov 9, 2016

emilio commented Nov 9, 2016

jdub commented Nov 9, 2016

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdub commented Nov 10, 2016

emilio commented Nov 10, 2016 • edited Loading

emilio commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

jdub commented Nov 8, 2016 •

edited

Loading

emilio commented Nov 10, 2016 •

edited

Loading