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

Translate _ as __ #1008

Merged
merged 1 commit into from Sep 21, 2017

Conversation

Projects
None yet
4 participants
@AndrewGaspar
Copy link
Contributor

AndrewGaspar commented Sep 21, 2017

This change treats _ as a reserved identifier to resolve the bug reported in #631.

I have one concern - if the header has both an _ and __ identifier in the global namespace, this will cause a conflict. However, it seems like we already don't handle that case for keyword_ (e.g. abstract_, alignof_, etc.) so it doesn't seem like we need a solution specifically for __ in this change.

Fixes #631.

@highfive

This comment has been minimized.

Copy link
Collaborator

highfive commented Sep 21, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@fitzgen
Copy link
Member

fitzgen left a comment

👍 thanks @AndrewGaspar !

@fitzgen

This comment has been minimized.

Copy link
Member

fitzgen commented Sep 21, 2017

I have one concern - if the header has both an _ and __ identifier in the global namespace, this will cause a conflict. However, it seems like we already don't handle that case for keyword_ (e.g. abstract_, alignof_, etc.) so it doesn't seem like we need a solution specifically for __ in this change.

We attempt to handle this sort of thing during the codegen phase, and have a map of method names and field names where we count how many times a name has been seen, and append that number to the name for disambiguation. So you get foo, foo1, foo2, foo3. etc.

It isn't perfect, but... shrug

@fitzgen

This comment has been minimized.

Copy link
Member

fitzgen commented Sep 21, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 21, 2017

📌 Commit aa11524 has been approved by fitzgen

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 21, 2017

⌛️ Testing commit aa11524 with merge 5b49548...

bors-servo added a commit that referenced this pull request Sep 21, 2017

Auto merge of #1008 - AndrewGaspar:underscore-identifier, r=fitzgen
Translate _ as __

This change treats _ as a reserved identifier to resolve the bug reported in #631.

I have one concern - if the header has both an `_` and `__` identifier in the global namespace, this will cause a conflict. However, it seems like we already don't handle that case for `keyword_` (e.g. `abstract_`, `alignof_`, etc.) so it doesn't seem like we need a solution specifically for `__` in this change.

Fixes #631.
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 21, 2017

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 5b49548 to master...

@bors-servo bors-servo merged commit aa11524 into rust-lang:master Sep 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@AndrewGaspar AndrewGaspar deleted the AndrewGaspar:underscore-identifier branch Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.