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

Re-introduce docopt #202

Closed
wants to merge 4 commits into from
Closed

Re-introduce docopt #202

wants to merge 4 commits into from

Conversation

jdub
Copy link
Contributor

@jdub jdub commented Nov 4, 2016

Fixes #87.

  • Tidy up usage string
  • Switch to using bindgen::Builder directly in main()
  • Builder required some new methods
  • --use-msvc-mangling appears to be deprecated, and is only referenced
    in src/lib.rs. I have added a Deprecated section to the usage string
    so using the flag doesn't cause errors
  • --emit-clang-ast doesn't currently disable output of Rust bindings,
    might have to do something tricky here
  • Added --version based on CARGO_PKG_*
  • Need to add some error handling to <input-header>

@jdub
Copy link
Contributor Author

jdub commented Nov 4, 2016

So,

  • Is --use-msvc-mangling no longer relevant? I can see bindgen is producing #[link_name] attributes with non-gcc-looking mangling. Must be detecting the target and doing the right thing? I can junk the remaining references in this PR if you like.
  • What is the appropriate behaviour for --emit-clang-ast? When passed, should it replace the Rust source entirely on stdout and --output?

Thanks. 😄

@jdub
Copy link
Contributor Author

jdub commented Nov 4, 2016

I'm not sure how to convince docopt to accept empty strings without an equals sign, e.g.

  • --raw-line '' as used in the tests fails (it takes the next parameter, even if it's a flag, as the string)
  • --raw-line='' works

@BurntSushi, suggestions?

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, only a few nits.

It'd be nice to have the --raw-line '' working, I'll reply to the other comments now.


// Input header
let header = args.get_str("<input-header>");
if header != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If !header.is_empty()? Here and below.

@emilio
Copy link
Contributor

emilio commented Nov 4, 2016

Is --use-msvc-mangling no longer relevant? I can see bindgen is producing #[link_name] attributes with non-gcc-looking mangling. Must be detecting the target and doing the right thing? I can junk the remaining references in this PR if you like.

Yep, I think it's unused since a while ago, and it works on MSVC, see #54. Let's get rid of it.

@emilio
Copy link
Contributor

emilio commented Nov 4, 2016

What is the appropriate behaviour for --emit-clang-ast? When passed, should it replace the Rust source entirely on stdout and --output?

This flag is intended only to be a debugging aid, so making it only spit to stdout is fine.

@bors-servo
Copy link

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

jdub added 4 commits November 5, 2016 09:09
Fixes rust-lang#87.

- Tidy up usage string
- Switch to using bindgen::Builder directly in main()
- Builder required some new methods
- `--use-msvc-mangling` appears to be deprecated, and is only referenced
in `src/lib.rs`. I have added a Deprecated section to the usage string
so using the flag doesn't cause errors
- `--emit-clang-ast` doesn't currently disable output of Rust bindings,
might have to do something tricky here
- Added `--version` based on CARGO_PKG_*
- Need to add some error handling to `<input-header>`
@jdub
Copy link
Contributor Author

jdub commented Nov 4, 2016

Interesting, @BurntSushi wants to move ripgrep to clap: BurntSushi/ripgrep#136

I'm going to let this PR cool and come back to it after the crate split discussed in #204. Then clap it. 👏

@bors-servo
Copy link

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

@jdub
Copy link
Contributor Author

jdub commented Nov 9, 2016

This PR has no future.

@jdub jdub closed this Nov 9, 2016
bors-servo pushed a commit that referenced this pull request Nov 10, 2016
Clap your hands say yeah! 👏🏻

In progress port to `clap`, obsoletes the `docopt` port in #202.
@jdub jdub deleted the docopt branch November 12, 2016 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants