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.

Sign up for GitHub

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

Jump to bottom

Remove some unnecessary symbol interner ops #61813

Merged
merged 2 commits into from
Jun 15, 2019
Merged

Remove some unnecessary symbol interner ops #61813

merged 2 commits into from
Jun 15, 2019

Conversation

matthewjasper
Copy link
Contributor

  • Don't gensym symbols that don't need to worry about colliding with other symbols
  • Use symbol constants instead of interning string literals in a few places.
  • Don't generate a module in __register_diagnostic

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2019
Centril
Centril reviewed Jun 13, 2019
View reviewed changes
src/librustc_resolve/lib.rs

fn intern(&mut self, string: &str, primitive_type: PrimTy) {
self.primitive_types.insert(Symbol::intern(string), primitive_type);
let mut table = FxHashMap::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a neat hashmap! { ... } macro in maplit that we could perhaps use to make this less boiler-platey?

petrochenkov
petrochenkov reviewed Jun 14, 2019
View reviewed changes
src/librustc_resolve/build_reduced_graph.rs
@@ -314,7 +314,7 @@ impl<'a> Resolver<'a> {
Ident::new(kw::SelfLower, new_span)
),
kind: ast::UseTreeKind::Simple(
Some(Ident::from_str_and_span("__dummy", new_span).gensym()),
Some(Ident::new(kw::Underscore, new_span)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this code is buggy (#61826), but this change shouldn't make it more buggy since the import is ty::Visibility::Invisible.

@petrochenkov
Copy link
Contributor

r=me with the nit addressed

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2019
@matthewjasper
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 14, 2019

📌 Commit 5c84cd3 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 14, 2019
@Centril
Rollup merge of rust-lang#61813 - matthewjasper:remove-unnecessary-sy…
5ca8640 
…mbol-ops, r=petrochenkov

Remove some unnecessary symbol interner ops

* Don't gensym symbols that don't need to worry about colliding with other symbols
* Use symbol constants instead of interning string literals in a few places.
* Don't generate a module in `__register_diagnostic`

r? @petrochenkov
@Centril Centril mentioned this pull request Jun 14, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jun 14, 2019
@Centril
Rollup merge of rust-lang#61813 - matthewjasper:remove-unnecessary-sy…
57822f9 
…mbol-ops, r=petrochenkov

Remove some unnecessary symbol interner ops

* Don't gensym symbols that don't need to worry about colliding with other symbols
* Use symbol constants instead of interning string literals in a few places.
* Don't generate a module in `__register_diagnostic`

r? @petrochenkov
@Centril Centril mentioned this pull request Jun 14, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jun 15, 2019
@Centril
Rollup merge of rust-lang#61813 - matthewjasper:remove-unnecessary-sy…
be09427 
…mbol-ops, r=petrochenkov

Remove some unnecessary symbol interner ops

* Don't gensym symbols that don't need to worry about colliding with other symbols
* Use symbol constants instead of interning string literals in a few places.
* Don't generate a module in `__register_diagnostic`

r? @petrochenkov
@Centril Centril mentioned this pull request Jun 15, 2019
Merged
bors added a commit that referenced this pull request Jun 15, 2019
@bors
Auto merge of #61868 - Centril:rollup-gglsecp, r=Centril
bb16e72 
Rollup of 6 pull requests

Successful merges:

 - #61785 (note some safety concerns of raw-ptr-to-ref casts)
 - #61805 (typeck: Fix ICE for blocks in repeat expr count.)
 - #61813 (Remove some unnecessary symbol interner ops)
 - #61824 (in which we decline to lint single-use lifetimes in `derive`d impls)
 - #61844 (Change `...` to `..=` where applicable)
 - #61854 (Minor cosmetic improvements to accompany PR 61825)

Failed merges:

r? @ghost
@bors bors merged commit 5c84cd3 into rust-lang:master Jun 15, 2019
@matthewjasper matthewjasper deleted the remove-unnecessary-symbol-ops branch July 29, 2019 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matthewjasper @petrochenkov @bors @Centril @rust-highfive