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

Handle templated aliases. #85

Merged
merged 7 commits into from
Oct 18, 2016
Merged

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Oct 14, 2016

Fixes #83.

cc @fitzgen
r? @nox

ty.name().map(ToOwned::to_owned)
.unwrap_or_else(|| format!("_bindgen_ty{}", self.id()))
// We really codegen and use the inner type, so use an empty
// base name so codegen doesn't get confused.
Copy link
Member

Choose a reason for hiding this comment

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

The start of this sentence has some funky grammar that prevents me from understanding it.

Is it supposed to be

We really want codegen to and use the inner type, ...

?

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 meant something like "We call codegen on the inner type, and since we use the parent chain to generate the canonical name, and we don't want this to appear, and can't be referenced itself from another type (we point to the actual alias for that), there's no need to generate a name for it".

Does that make sense?

Copy link
Member

@fitzgen fitzgen Oct 14, 2016

Choose a reason for hiding this comment

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

Thanks, I think I understand much better now. I would phrase it like this:

We call codegen on the inner type, but we do not want this alias's name to appear in the canonical name just because it is in the inner type's parent chain, so we use an empty base name.

How is that?

(Thanks for painting this bike shed with me)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll update the comment. And no problem, this is one of the trickiest things in the new bindgen rewrite I think :)

@@ -0,0 +1,13 @@
// bindgen-flags: --enable-cxx-namespaces -- -std=c++14
Copy link
Member

Choose a reason for hiding this comment

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

What happens if we don't have --enable-cxx-namespaces here? Will the generated rust code be all in the same namespace, or will it fail, or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the exact same test that tests/headers/template_alias.hpp, just that flag changes, so you can see the output right away :)

Copy link
Member

Choose a reason for hiding this comment

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

D'oh this was exactly what I was looking for :P

Not sure how I missed that...

@bors-servo
Copy link

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

@nox
Copy link
Contributor

nox commented Oct 18, 2016

@emilio r=me once it is rebased.

@emilio
Copy link
Contributor Author

emilio commented Oct 18, 2016

@bors-servo: r=nox

@bors-servo
Copy link

📌 Commit 2a3f930 has been approved by nox

@bors-servo
Copy link

⌛ Testing commit 2a3f930 with merge ab6c438...

bors-servo pushed a commit that referenced this pull request Oct 18, 2016
Handle templated aliases.

Fixes #83.

cc @fitzgen
r? @nox
@bors-servo
Copy link

☀️ Test successful - status-travis

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

5 participants