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

full support non rust naming conventions of generated code #580

Closed
wants to merge 8 commits into from
Closed

full support non rust naming conventions of generated code #580

wants to merge 8 commits into from

Conversation

framlog
Copy link
Contributor

@framlog framlog commented Mar 13, 2017

No difference with current version if c++ namespace is enabled. But it generates a root mod to wrap all codes otherwise.

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

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.

I have a few concerns about the complexity that this introduces.

Also, as I said before, it seems like a too breaking change to the generated code for a really small benefit...

@@ -360,6 +364,12 @@ impl CodeGenerator for Module {
}
}

if item.id() == ctx.sentinel_module() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole sentinel_module thing really feels like a hack. Is it just for this? If so, if you always want this at the top of the file, let's just move its handling above.

Copy link
Contributor Author

@framlog framlog Mar 13, 2017

Choose a reason for hiding this comment

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

@emilio yeah, sentinel_module is only for this usage. But I don't think I could move these statements to some other place.

}
};

if !ctx.options().enable_cxx_namespaces ||
let name = item.canonical_name(ctx);
if name == "<sentinel>" || (name != "root" && !ctx.options().enable_cxx_namespaces) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Just check the item_id?

@@ -131,7 +131,8 @@ pub fn cursor_mangling(ctx: &BindgenContext,
}

// Try to undo backend linkage munging (prepended _, generally)
if cfg!(target_os = "macos") {
if get_abi(cursor.cur_type().call_conv()).map_or(true, |abi| abi != abi::Abi::Stdcall) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This only seems to be working around a test expectation difference on OSX while running with -target msvc, but this is not a proper fix.

The difference happen because this cfg! check is on the target that compiled bindgen, not on the target that clang is parsing files as. That's a totally unrelated fix, and you should just skip adding that file to the diff and revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think its a fix either. In fact, I don't fully understand the purport of this if statement since I got the proper names on macos when the calling convention is stdcall or cdecl or whatever... I'll remove this line.

src/lib.rs Outdated
@@ -792,6 +792,10 @@ impl<'ctx> Bindings<'ctx> {
try!(writer.write("\n".as_bytes()));
}

if !self.context.options().enable_cxx_namespaces && self.module.items.len() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: !self.module.items.is_empty().

@framlog
Copy link
Contributor Author

framlog commented Mar 13, 2017

Thanks for reviewing. I think it's just a small hack except one should pay little attention to handle root mod when cxx namespace is enabled...

@bors-servo
Copy link

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

@bors-servo
Copy link

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

@bors-servo
Copy link

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

@fitzgen
Copy link
Member

fitzgen commented Apr 6, 2017

@framlog hi :)

Are you still planning on rebasing this and addressing @emilio's feedback?

@framlog
Copy link
Contributor Author

framlog commented Apr 7, 2017

@fitzgen sry, I thought I've done.... I'll fix this

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.

I'm still not totally happy about the code generation results, but I guess it's fine... Still a few changes needed though.

src/lib.rs Outdated
@@ -826,6 +826,10 @@ impl<'ctx> Bindings<'ctx> {
try!(writer.write("\n".as_bytes()));
}

if !self.context.options().enable_cxx_namespaces && !self.module.items.is_empty() {
try!(writer.write("pub use self::root::*;\n\n".as_bytes()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use write_str here.

if !ctx.options().enable_cxx_namespaces ||
(self.is_inline() && !ctx.options().conservative_inline_namespaces) {
let name = item.canonical_name(ctx);
if name == "<sentinel>" || (name != "root" && !ctx.options().enable_cxx_namespaces) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check the module id's instead of the names.

@@ -1258,7 +1265,7 @@ impl CodeGenerator for CompInfo {
quote_ty!(ctx.ext_cx(), __BindgenUnionField<$ty>)
}
} else if let Some(item) =
field_ty.is_incomplete_array(ctx) {
field_ty.is_incomplete_array(ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the rustfmt changes so this is easier to review.

fitzgen and others added 8 commits April 8, 2017 08:21
This replaces various `unwrap` calls with `expect` calls that have better
diagnostic messages if/when they fail.
This commit ensures that all of the cargo features we have that only exist for
CI/testing purposes, and aren't for external consumption, have a "testing_only_"
prefix.
This commit defines a new set of assertion macros that are only checked in
testing/CI when the `testing_only_extra_assertions` feature is enabled. This
makes it so that *users* of bindgen that happen to be making a debug build don't
enable all these extra and expensive assertions.

Additionally, this removes the `testing_only_assert_no_dangling_items` feature,
and runs the assertions that were previously gated on that feature when the new
`testing_only_extra_assertions` feature is enabled.
@framlog
Copy link
Contributor Author

framlog commented Apr 11, 2017

r?@emilio

@jdm
Copy link
Contributor

jdm commented May 23, 2017

@emilio Will you have time to get back to this?

@emilio
Copy link
Contributor

emilio commented May 24, 2017

So the thing is I'm still not quite sure this is the right trade off, both in terms of the complexity it introduces into bindgen, and in the generated output.

I still think it's quite easy to just use the raw_line API so that each person can adapt bindgen to their needs.

Now we have a tools team, so I guess we could discuss this, wdyt @fitzgen?

@fitzgen
Copy link
Member

fitzgen commented Aug 23, 2017

I think we've kind of come to the conclusion that this isn't worth the trouble, and its pretty out of date now.

Going to close it and we can open new issues/PRs for anything more in the future

@fitzgen fitzgen closed this Aug 23, 2017
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.

7 participants