Include namespaces in mangled symbols #281
Conversation
|
\o/ awesome! Will review this properly tomorrow, and probably run it through stylo since the change is likely breaking, but awesome! :) Thanks for doing this :) |
There was a problem hiding this comment.
Looks great, though this means that the stylo script probably should change some of its whitelisting and blacklisting. cc @Manishearth and @upsuper.
| // Template aliases use their inner alias type's name if we | ||
| // are checking names for whitelisting/replacement/etc. | ||
| TypeKind::TemplateAlias(inner, _) if for_name_checking => { | ||
| item = ctx.resolve_item(inner); |
There was a problem hiding this comment.
probably should be able to just return inner in this case.
There was a problem hiding this comment.
Again, will try with a debug_assert!.
| // If we're a template specialization, our name is our | ||
| // parent's name. | ||
| TypeKind::Comp(ref ci) if ci.is_template_specialization() => { | ||
| item = ctx.resolve_item(ci.specialized_template().unwrap()); |
There was a problem hiding this comment.
and here we may be able to just do return ci.specialized_template(), this can't be recursive if I'm not wrong.
There was a problem hiding this comment.
I'll do this, but also add a debug_assert! that the specialized template's name target is itself.
There was a problem hiding this comment.
This assertion is failing in our tests, I'm going to leave the code as it was.
There was a problem hiding this comment.
Just curious, which tests are failing?
|
|
||
| /// Get this function item's name, or `None` if this item is not a function. | ||
| fn func_name(&self) -> Option<&str> { | ||
| if let ItemKind::Function(ref func) = *self.kind() { |
There was a problem hiding this comment.
Using match you can save a line, but at your choice :)
|
|
||
| /// Get the overload index for this method. If this is not a method, return | ||
| /// `None`. | ||
| fn method_overload_index(&self, ctx: &BindgenContext) -> Option<usize> { |
There was a problem hiding this comment.
Ahh, I understand why this can't be completely moved to codegen, we generate calls for it... Well looks fine :)
When we aren't using `--enable-cxx-namespaces`, we can end up with conflicting struct symbol names that we need to disambiguate. The solution is to mangle the namespaced C++ symbol "foo::bar::Baz" into the Rust "foo_bar_Baz" symbol.
fitzgen
force-pushed
the
namespace-mangle-rust-symbols
branch
from
cfbb7d4 to
2a93411
Compare
|
r? @emilio, I addressed the previous review's comments. |
|
@bors-servo r+ |
|
📌 Commit 2a93411 has been approved by |
|
⚡ Test exempted - status |
Include namespaces in mangled symbols When we aren't using `--enable-cxx-namespaces`, we can end up with conflicting struct symbol names that we need to disambiguate. The solution is to mangle the namespaced C++ symbol "foo::bar::Baz" into the Rust "foo_bar_Baz" symbol. This did change the way anonymous types and modules get named a little, but I think our approach is much more sane now than it was before. Fixes #267. r? @emilio
|
THanks @emilio! :) |
|
This busts stylo bindings, causing us to generate erroneous code like: Is this related to the whitelisting/blacklisting @emilio was mentioning above? In general, it would be nice if large changes like this were tested against stylo before landing. |
When we aren't using
--enable-cxx-namespaces, we can end up withconflicting struct symbol names that we need to disambiguate. The
solution is to mangle the namespaced C++ symbol "foo::bar::Baz" into the
Rust "foo_bar_Baz" symbol.
This did change the way anonymous types and modules get named a little,
but I think our approach is much more sane now than it was before.
Fixes #267.
r? @emilio