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

automatically allow non rust naming conventions #565

Merged
merged 1 commit into from
Mar 9, 2017
Merged

automatically allow non rust naming conventions #565

merged 1 commit into from
Mar 9, 2017

Conversation

framlog
Copy link
Contributor

@framlog framlog commented Mar 7, 2017

I just added those attributes at the root mod. And I'm not sure whether it should be better if we could set this setting in build.rs.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

We need to sort out the mangling issue before this can land, however.

@@ -37,6 +37,6 @@ impl Default for JNINativeInterface_ {
fn default() -> Self { unsafe { ::std::mem::zeroed() } }
}
extern "stdcall" {
#[link_name = "_bar@0"]
#[link_name = "bar@0"]
Copy link
Member

Choose a reason for hiding this comment

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

This change is failing in CI. @emilio do you know what's up with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is due to us not testing on OSX (where we remove an extra underscore from symbols that clang adds). Of course, this is cross-compiled so a cfg check is not enough. Let's just remove this change from the PR for now.

@fitzgen
Copy link
Member

fitzgen commented Mar 7, 2017

And I'm not sure whether it should be better if we could set this setting in build.rs.

We can wait and see if anyone cares enough about these warnings to file an issue requesting configurability :)

@emilio
Copy link
Contributor

emilio commented Mar 9, 2017

@fitzgen is this ready to land? Looks ok to me.

@fitzgen
Copy link
Member

fitzgen commented Mar 9, 2017

Yeah, I didn't get an email about the new commit for whatever reason.

@bors-servo r+

Thanks @framlog! If you'd like to continue hacking on bindgen, I can point you at some more things :) Thanks again!

@bors-servo
Copy link

📌 Commit e0ca632 has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit e0ca632 with merge 1320efe...

bors-servo pushed a commit that referenced this pull request Mar 9, 2017
automatically allow non rust naming conventions

+ related issue: #562

I just added those attributes at the root mod. And I'm not sure whether it should be better if we could set this setting in `build.rs`.
@fitzgen
Copy link
Member

fitzgen commented Mar 9, 2017

I guess one potential issue is that this will only work if one has enabled C++ namespaces, otherwise we won't generate the module definition and its allow attributes.

@bors-servo
Copy link

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

@bors-servo bors-servo merged commit e0ca632 into rust-lang:master Mar 9, 2017
@framlog
Copy link
Contributor Author

framlog commented Mar 10, 2017

@fitzgen ok, I'd like to do more work on it. thx

@fitzgen
Copy link
Member

fitzgen commented Mar 10, 2017

@fitzgen ok, I'd like to do more work on it. thx

I'd suggest looking into how we prepend raw lines to the bindings, and inserting #![allow(...)] there. The raw lines get inserted regardless whether we are enabling C++ namespaces or not. Try grepping for raw_lines to get an idea what I'm talking about.

If anything doesn't make sense, don't hesitate to ask a question :)

@framlog
Copy link
Contributor Author

framlog commented Mar 10, 2017

@fitzgen Inserting an attribute at the beginning of a generated file is easy. However, I can't find a way to make the generated file can work in integration if I just insert that line. (Seems that rustc doesn't allow include! inner attributes in a mod) Could I generate a top mod if enabling C++ namespaces are not set?

@emilio
Copy link
Contributor

emilio commented Mar 10, 2017

I'd suggest looking into how we prepend raw lines to the bindings, and inserting #![allow(...)] there. The raw lines get inserted regardless whether we are enabling C++ namespaces or not. Try grepping for raw_lines to get an idea what I'm talking about.

I'd be hesitant to allow that by default, it could break legit usages of include!.

@fitzgen
Copy link
Member

fitzgen commented Mar 10, 2017

Ah, good catch, I didn't think about how it would play out inside an include!.

@framlog
Copy link
Contributor Author

framlog commented Mar 11, 2017

Well, I managed to allow non rusty conventions by wrapping the whole generated code in a root mod regardless of enabling c++ namespaces. It doesn't bother the integration I think. And I could make a pr after I fixing the name mangling issue if you think it's ok.

@emilio
Copy link
Contributor

emilio commented Mar 11, 2017

I guess that's fair, though that's a much more breaking change...

@framlog
Copy link
Contributor Author

framlog commented Mar 11, 2017

ok, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants