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

libsyntax: Do not derive Hash for Ident #28823

Merged
merged 2 commits into from
Oct 6, 2015
Merged

Conversation

petrochenkov
Copy link
Contributor

Closes #28658

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned pnkfelix Oct 4, 2015
@@ -35,7 +35,7 @@ use std::collections::HashMap;
pub struct SCTable {
table: RefCell<Vec<SyntaxContext_>>,
mark_memo: RefCell<HashMap<(SyntaxContext,Mrk),SyntaxContext>>,
rename_memo: RefCell<HashMap<(SyntaxContext,Name,SyntaxContext,Name),SyntaxContext>>,
rename_memo: RefCell<HashMap<(SyntaxContext,(Name,SyntaxContext),Name),SyntaxContext>>,
Copy link
Member

Choose a reason for hiding this comment

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

Why make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for clarity. It's actually an Ident (and it was literally Ident before #28642), but it should be hashed and compared as pair and not as an Ident.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining this please?

@nrc
Copy link
Member

nrc commented Oct 5, 2015

This seems like a good change, but I wonder if we should be hashing idents at all - do we need the hash implementation?

@petrochenkov
Copy link
Contributor Author

I wonder if we should be hashing idents at all - do we need the hash implementation?

All larger structures in AST/HIR need it to derive their Hashes, but... all these hashes are never actually used with exception of one small piece of SVH, where its use is a bug. So, yes, implementations of Hash can be removed from Ident and dozens of other structures including it.

@nrc
Copy link
Member

nrc commented Oct 5, 2015

So, yes, implementations of Hash can be removed from Ident and dozens of other structures including it.

I assume you want to do this as a follow-up, not here?

@nrc
Copy link
Member

nrc commented Oct 5, 2015

r+ with the comment

@petrochenkov
Copy link
Contributor Author

I assume you want to do this as a follow-up, not here?

I wasn't sure if it was needed to be done, maybe third party plugins or tools hash parts of AST or who knows, but if removing these hashes is ok, then yes, I'd prefer to do it later.
Updated with a comment for mtwt.

@nrc
Copy link
Member

nrc commented Oct 6, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 6, 2015

📌 Commit b82d76c has been approved by nrc

@bors
Copy link
Contributor

bors commented Oct 6, 2015

⌛ Testing commit b82d76c with merge b1d9ce9...

bors added a commit that referenced this pull request Oct 6, 2015
@bors bors merged commit b82d76c into rust-lang:master Oct 6, 2015
@petrochenkov petrochenkov deleted the identeq2 branch November 22, 2015 11:45
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

6 participants