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

Improve tracing so it doesn't ignore opaque stuff when it shouldn't #387

Merged
merged 4 commits into from Jan 11, 2017

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Jan 7, 2017

Concretely, trace through functions, pointers, arrays, references, resolved type references, and template references, which are the types for which either:

  • There's no special meaning to be opaque, or
  • They're just placeholders that point to other types.

@emilio
Copy link
Contributor Author

emilio commented Jan 7, 2017

r? @fitzgen

Needs a test, but should be straight forward.

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.

Thanks! LGTM, but I have a couple comments below.

"{" => None,
s => Some(s.to_owned()),
let mut module_name = None;
if let Some(tokens) = self.translation_unit.tokens(&cursor) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be absolutely lovely if this tokens processing wasn't inline, and instead was a separate helper function.

fn is_inline_namespace(unit: &clang::TranslationUnit, cursor: &clang::Cursor) -> bool {
    ...
}

namespace foo {
inline namespace bar {
using Ty = int;
};
Copy link
Member

Choose a reason for hiding this comment

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

What are the C++ semantics for when there are conflicts between the inline namespace and its parent namespace? If it is permitted at all, and not a compilation error, then we should test that we Do The Right Thing in this scenario.

namespace foo {
  inline namespace bar {
    using Ty = int;
  }
  using Ty = char;
}

http://stackoverflow.com/questions/24220127/redefinition-member-of-the-namespace-into-the-nested-inline-namespace#24220504 seems relevant

Copy link
Member

Choose a reason for hiding this comment

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

Also, what about multiple nested inline namespaces? Seems like another good thing to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, that's a good question. Multiple nested inline namespaces work fine with this patch.

Overriding an item defined in an inline namespace is legal, though referencing it is a compile error.

If we want to support this edge case (which I think is kinda pointless tbh), we should keep all the namespaces in Rust I think. That will make bindgen harder to use with the STL though, given you no longer have std::string, but std::__gnu_xxx::string, etc.

This deserves a bit more discussion so I'll move this to another PR.

@emilio
Copy link
Contributor Author

emilio commented Jan 11, 2017

Dropped the inline namespace commit, and added a test.

@bors-servo r=fitzgen

@bors-servo
Copy link

📌 Commit 66447ff has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit 66447ff with merge 54aba18...

bors-servo pushed a commit that referenced this pull request Jan 11, 2017
Improve tracing so it doesn't ignore opaque stuff when it shouldn't

Concretely, trace through functions, pointers, arrays, references, resolved type references, and template references, which are the types for which either:

 * There's no special meaning to be opaque, or
 * They're just placeholders that point to other types.
@bors-servo
Copy link

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit 66447ff into rust-lang:master Jan 11, 2017
jethrogb pushed a commit to jethrogb/rust-bindgen that referenced this pull request Oct 18, 2019
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.

None yet

4 participants