merge into upstream!

[Yamakaky]

Did you consider to merge your modifications into my repo? We both added a lot of features, it would be great if we could join forces.

[Manishearth]

We'd love to. Some of the modifications might be servo-specific though, so we should be careful when merging.

[metajack]

We've consolidated all the forks we knew about in servo/rust-bindgen. I don't think we were aware of yours.

It would be highly preferable for there to be a single rust-bindgen. If it looks possible to consolidate these, I think we'd love to do it. Perhaps we should ask the Rust team if this is appropriate to move to rustlang/bindgen or something more official.

[Yamakaky]

In fact, crabtw transfered me the ownership since he was inactive since a few months and I was the only commiter ^^

[metajack]

Huh. I thought we had asked crabtw to transfer it to us a long time ago and he preferred not to do that. In any case, it's all water under the bridge and we should try and get down to a single bindgen if at all possible.

[Yamakaky]

I agree. What do you suggest?

[metajack]

Someone needs to look at the commit history since the forks diverged and see how much work it is to unify, and then we need to do the unification.

And then we need to decide where the official repo will be, and then recreate personal forks to be from that one.

Finally, we'll need to hook up CI and decide on reviewers and such. I think there are approximately 3-4 people on the Servo team who've been contributing to bindgen.

The main piece of work is the first part. The rest is relatively trivial.

[emilio]

So I tried the "YOLO rebase the world on top of upstream approach" a while ago, and it didn't work as well as I'd expected, there are a lot of non-trivial changes, so I think the way to go is upstreaming features individually. But that's a really time consuming thing to do.

Probably one of the easiest part that would simplify a lot upstreaming more features is upstreaming the types.rs changes first, since that file hasn't been modified a lot upstream.

On top of that, I think the best approach is porting our tests, and making them pass one at a time, since there are a few tricky cases there. I'd love to do this, but unfortunately I don't think I'll have the time to do it near-term.

Nonetheless, does that approach sound good to you?

[Yamakaky]

yeah, yolo is not a good approch in this case ^^
I also think it would be easier. We can also start with clang.rs.
BTW, I replaced the manual argument parsing with docopt, and clangll.rs by the crate clang-sys.
I will start by integrating types.rs in my repo.

[Yamakaky]

in fact, WIP on clang.rs

[Yamakaky]

Just finished clang.rs. It changes the supported clang version requirement:

= 3.6: clang_Cursor_getTemplateArgumentValue and Kind, clang_Cursor_getMangling
= 3.8: clang_getCursorVisibility, clang_CXXField_isMutable
= 3.9: clang_Type_getNamedType, clang_Cursor_isFunctionInlined
WIP KyleMayes/clang-sys#35 for all the HTML and Comment functions.

See http://llvm.org/releases/. Ok for 3.6, but 3.8 was released on march and 3.9 even after, so it may not be too supported. Are these functions really needed?

[emilio]

That's one of the reasons we haven't switched to clang-sys completely. We need to be able to compile on stable clang, but llvm 3.9 is needed for SpiderMonkey bindings.

We use conditional compilation for that, and it seems to me that clang-sys have added support for that, so we could keep compiling conditionally.

[Yamakaky]

Could you list the features that needs clang 3.9?

[emilio]

Basically the ability to detect if a function is inlined and don't generate bindings for these, because they generated missing symbols.

The other 3.9 function is the named type thing, which is a backwards incompatible change from clang that was needed to fix bindgen on llvm 3.9

[emilio]

In #31 I'm adding the last test from upstream that isn't in our test suite. @Yamakaky, would you consider making this bindgen the canonical one? I think there's no situation yours can handle that ours doesn't, and even though we lose some things, like the change from manual option parsing to docopt, I don't consider those a priority right now.

[Yamakaky]

Did you go through https://github.com/Yamakaky/rust-bindgen/blob/master/Changelog.md to check?
Also, your --help is outdated, it misses all the switches you added. With docopt, it would not be a problem. The change is not hard (Yamakaky@b6f9f95).
And did you consider using clang_sys instead of clangll.rs?

[emilio]

@Yamakaky FWIW, tried docopt, doesn't scale well for our use case (#46). If we don't find a solution relatively early, I'll have to back it out, since it's unusable for our purposes.

[upsuper]

FWIW, we need 3.9 for generating Windows bindings, because Clang 3.8 doesn't parse MSVC2015's headers properly. Nevertheless, 3.9 has been released :)

[Manishearth]

Not yet in brew though.

[Yamakaky]

Going back to it!

I really want us to merge, especially since your refactoring. I'll try to open issues about features missing in your fork. The main problem I have is the dependency to clang 3.8... Do you think it would be reasonable to use conditional compilation to reduce this version, like disabling c++ support?

BTW, would it be possible to add me as a maintainer?

[jethrogb]

I think there's no situation yours can handle that ours doesn't

This fork does not support expression parsing in macros.

[emilio]

Fair enough, there you go: #219 :)

[Yamakaky]

@jethrogb did you find other things to merge?

[jethrogb]

@jethrogb did you find other things to merge?

Just taking a quick look at https://docs.rs/bindgen/0.19.1/bindgen/struct.Builder.html and the same page generated from this fork, there are a bunch of missing features.

[emilio]

So most of them, at least as far as I know, are intentionally
removed/replaced, or with an implementation pending.

For example:

• override_enum_ty: Intentionally removed since it breaks layouts of
  types. Happy to add it back if a use case arises.
• use_core and ctypes_prefix: PR open awaiting review.
• convert_floats: Had not thought about it, but sounds reasonable,
  and trivially easy to implement.
• use_macro_types: In general I'm dubious having different types
  depending on the macro range by default is useful, for example,
  something like bitflags, where the bitflags have different types as
  you go up. Seems reasonable to add another kind of API for it though.
• remove_prefix: Slightly easy to implement, though I'm not sure it's
  super-useful. We have enough naming shenanigans to support C++, but
  seems like it could be an easy addition if needed.

[jethrogb]

What about:

• rust_enums: As clang-sys recently found out the hard way, Rust enums are not the same thing as C enums. C enums can hold any value that fits the type but the Rust compiler might generate crashing code if the value is unexpected. The only somewhat safe way to handle this is by using integer types instead of Rust enums.
• match_pat?
• derive_debug?

[jethrogb]

use_macro_types: In general I'm dubious having different types
depending on the macro range by default is useful, for example,
something like bitflags, where the bitflags have different types as
you go up. Seems reasonable to add another kind of API for it though.

Yeah I really don't know what the best way to handle this would be. You really need more context to say anything about these. As I mentioned in #219 some kind of post-processing could work. Alternatively, the user could pass in a closure that's used for determining the type.

[Yamakaky]

https://github.com/andersk/enum_primitive-rs

[emilio]

It seems that the API I added over in #219 is going to help with the macro type thing.

@jethrogb:

• match_pat: This is replaced by the whitelisting APIs.
• rust_enums: I kind of understand the reasoning for that, but at that point, there's no point in having an enum itself, but an integer type, and moving from the enum variant you want to integer. An API can be added for that, but I'm not sure it's worth.
• derive_debug: We always derive debug when we can, and haven't found a case when its incorrectly derived, something that with the old bindgen was extremely hard to do. Unless there's a reason to explicitly disable it, there's no point in adding a switch for it I think.

[jethrogb]

match_pat: This is replaced by the whitelisting APIs.

That's not the same because it doesn't work at the file level. If I understand correctly, with that API I need to add about 1600 whitelisted items plus I need to go through all the changes everytime there is an upstream update to add more?

rust_enums: there's no point in having an enum itself, but an integer type, and moving from the enum variant you want to integer

This is exactly what rust_enums(false) does.

derive_debug: We always derive debug when we can, and haven't found a case when its incorrectly derived

Ok. It was buggy in upstream but maybe you've fixed everything in that regard :)

[emilio]

That's not the same because it doesn't work at the file level. If I understand correctly, with that API I need to add about 1600 whitelisted items plus I need to go through all the changes everytime there is an upstream update to add more?

Items are whitelisted recursively, so in practice it's not bad.

[jethrogb]

From mbedtls-sys-auto:

    841 pub const
    684 pub fn
     73 pub struct
     44 pub type

I don't think recursiveness is going to help reducing those const and fn numbers.

[emilio]

You can specify regexes while whitelisting. That should help for any API that is midly consistent.

[emilio]

Indeed, what we do for Gecko is --whitelist-function "Servo_.*", to whitelist all our functions, for example.

[emilio]

That being said, probably that should be better documented.

[jethrogb]

Oh. That's not at all clear from the documentation indeed. That would probably work for me (depending on the interaction with remove_prefix)

[emilio]

So I think with the last support for bitfield-like enums I landed in #226, the only pending stuff would be remove_prefix, is that right @jethrogb?

[jethrogb]

I'll try to run this on mbedTLS sometime this weekend to check. I'm using a lot of bindgen features there but I can't guarantee I'm using all of them, so don't assume my blessing is enough :)

[tupshin]

FWIW, this works as a near drop-in replacement for the crabtw version for my cassandra-sys-rs project.
And most of those changes were me removing code now that bindgen did a better job at deriving types. My only blocker now is that this isn't on crates.io. :)

[emilio]

Glad to hear that! :)

FWIW it is now on crates.io:

https://crates.io/crates/libbindgen

[tupshin]

oh i totally missed that. thanks so much.

[jethrogb]

Sorry for the flood of issues that are coming in now but I haven't had time yet to test this out...

Remaining regressions: #426, #427, #428, #429, #430, #431, #432