Skip to content

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Nov 15, 2016

When we aren't using --enable-cxx-namespaces, we can end up with
conflicting struct symbol names that we need to disambiguate the same
way we disambiguate overloaded functions.

Fixes #267.

r? @emilio

When we aren't using `--enable-cxx-namespaces`, we can end up with
conflicting struct symbol names that we need to disambiguate the same
way we disambiguate overloaded functions
@fitzgen
Copy link
Member Author

fitzgen commented Nov 15, 2016

Again, canceling this CI build until the one that fixes Travis CI lands, which is taking forever...

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.

This wont work reasonably well I fear.

let canonical_name = item.canonical_name(ctx);

let mut canonical_name = item.canonical_name(ctx);
let times_seen = result.overload_number(&canonical_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

For function overloads this is fine, but for structs it's not, since we can't disambiguate them when we refer to them. I think this logic belongs to canonical_name, and indeed I think it worked at some point (see the namespacing and parent_canonical_name logic), it just needs to be tuned so it works again.

@emilio
Copy link
Contributor

emilio commented Nov 15, 2016

I think the culprit to fix this is fixing is_toplevel to check a different thing in real_canonical_path to look at the parent name more often.

@fitzgen
Copy link
Member Author

fitzgen commented Nov 15, 2016

I think the culprit to fix this is fixing is_toplevel to check a different thing in real_canonical_path to look at the parent name more often.

What do you suggest checking instead? I almost feel like we shouldn't allow C++ namespaces to be optional... Although that would require even more refactoring in SM bindings...

@emilio
Copy link
Contributor

emilio commented Nov 15, 2016

@fitzgen my point is that we use is_toplevel for two different things, one for which we want to ignore namespaces if they're not enabled (whitelisting, because if we don't consider them toplevel they won't get generated), and one for which we don't (naming, because if we do consider them toplevel we don't mangle the name correctly).

@fitzgen fitzgen closed this Nov 18, 2016
luser pushed a commit to luser/rust-bindgen that referenced this pull request Mar 27, 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.

3 participants