-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Removed 'proc' from the reserved keywords list #49699
Conversation
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Merge upstream changes
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Thanks for your PR! We'll make sure to get a top-notch reviewer on the case as soon as possible! Randomly assigning... |
@bors r+ |
📌 Commit 5e94d54 has been approved by |
⌛ Testing commit 5e94d54 with merge 58ba05d572989b1c57151b256de4c164befe88fd... |
💔 Test failed - status-travis |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@@ -318,7 +318,6 @@ declare_keywords! { | |||
(46, Offsetof, "offsetof") | |||
(47, Override, "override") | |||
(48, Priv, "priv") | |||
(49, Proc, "proc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keyword list shouldn't contain holes (otherwise random identifiers will be interpreted as keywords), so the numbers of following keywords have to be shifted by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch. @zesterer?
(Also, I wonder if we could construct this list in another way so that it auto-numbers, eliminating this issue in the future.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshtriplett My experience with the compiler codebase is less than excellent. Is this the sort of thing that could be easily macro-driven?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only keyword removed in the last 3-4 years, certainly not something that need to be automated by extra macro machinery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrochenkov I may be over-thinking this. Is this literally just a case of enumerating everything that comes after the removed keyword?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zesterer
Yes
Should we perhaps go through approval from all of lang team before merging this? Seems like a semi-major change (removing a keyword)... |
You said you wanted to be included in this, @nikomatsakis ? |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Isn't it good to note that this change is based on #19338? |
Ping from triage @joshtriplett! The author addressed the comments in this PR, could you review it again? |
Thanks for the updates; LGTM. @bors r+ |
📌 Commit 7f58d2f has been approved by |
@bors rollup |
Removed 'proc' from the reserved keywords list Remove 'proc' from the reserved keywords list. 'proc' is a very useful identifier name for a lot of things. It's especially useful when dealing with processes, operating system internals, and kernel development.
cc @rust-lang/lang @Centril This PR really needed an RFC, or at the very least an FCP step from the lang team (AFAIK the change has not been approved elsewhere). Not sure whether we want to take further action here? |
I can have an RFC ready tomorrow :) |
This was removed in rust-lang/rust#49699
Remove 'proc' from the reserved keywords list.
'proc' is a very useful identifier name for a lot of things. It's especially useful when dealing with processes, operating system internals, and kernel development.