Use proc-macro to derive HashStable everywhere #66279
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job 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 |
|
The job 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 |
|
☔ The latest upstream changes (presumably #66252) made this pull request unmergeable. Please resolve the merge conflicts. |
cjgillot
force-pushed
the
hashstable
branch
2 times, most recently
from
3a293d9
to
031a359
Compare
|
☔ The latest upstream changes (presumably #60026) made this pull request unmergeable. Please resolve the merge conflicts. |
|
There's some commits which only adds |
Zoxc
reviewed
Nov 15, 2019
|
You can also create another PR where you introduce the |
|
☔ The latest upstream changes (presumably #66453) made this pull request unmergeable. Please resolve the merge conflicts. |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Nov 20, 2019
Just derive Hashstable in librustc Split out of rust-lang#66279 r? @Zoxc
Centril
added a commit
to Centril/rust
that referenced
this pull request
Nov 20, 2019
Add a proc-macro to derive HashStable in librustc dependencies A second proc-macro is added to derive HashStable for crates librustc depends on. This proc-macro HashStable_Generic (to bikeshed) allows to decouple code and some librustc's boilerplate. Not everything is migrated, because `Span` and `TokenKind` require to be placed inside librustc. Types using them stay there too. Split out of rust-lang#66279 r? @Zoxc
Centril
added a commit
to Centril/rust
that referenced
this pull request
Nov 21, 2019
Add a proc-macro to derive HashStable in librustc dependencies A second proc-macro is added to derive HashStable for crates librustc depends on. This proc-macro HashStable_Generic (to bikeshed) allows to decouple code and some librustc's boilerplate. Not everything is migrated, because `Span` and `TokenKind` require to be placed inside librustc. Types using them stay there too. Split out of rust-lang#66279 r? @Zoxc
Centril
added a commit
to Centril/rust
that referenced
this pull request
Nov 21, 2019
Add a proc-macro to derive HashStable in librustc dependencies A second proc-macro is added to derive HashStable for crates librustc depends on. This proc-macro HashStable_Generic (to bikeshed) allows to decouple code and some librustc's boilerplate. Not everything is migrated, because `Span` and `TokenKind` require to be placed inside librustc. Types using them stay there too. Split out of rust-lang#66279 r? @Zoxc
bors
added a commit
that referenced
this pull request
Nov 22, 2019
Add a proc-macro to derive HashStable in librustc dependencies A second proc-macro is added to derive HashStable for crates librustc depends on. This proc-macro HashStable_Generic (to bikeshed) allows to decouple code and some librustc's boilerplate. Not everything is migrated, because `Span` and `TokenKind` require to be placed inside librustc. Types using them stay there too. Split out of #66279 r? @Zoxc
|
Rebased. |
Zoxc
reviewed
Nov 23, 2019
| fn hash_stable(&self, hcx: &mut CTX, hasher: &mut StableHasher) { | ||
| self.segments.len().hash_stable(hcx, hasher); | ||
| for segment in &self.segments { | ||
| segment.ident.name.hash_stable(hcx, hasher); |
There was a problem hiding this comment.
@michaelwoerister This skips all the other fields in PathSegment and also the span field in Path. Is this intentional?
There was a problem hiding this comment.
It looks like the NodeId field was added later and the whole implementation was lifted to a more general context in 759bd01. It would be good to hash the generic arguments too.
|
I would like to get rid of the |
Zoxc
reviewed
Nov 23, 2019
Zoxc
reviewed
Nov 23, 2019
Zoxc
reviewed
Nov 23, 2019
Zoxc
reviewed
Nov 23, 2019
| where CTX: crate::StableHashingContextLike | ||
| { | ||
| fn hash_stable(&self, hcx: &mut CTX, hasher: &mut StableHasher) { | ||
| for sub_tt in self.trees() { |
There was a problem hiding this comment.
@nnethercote It seems like this iterator does a bunch of Lrc clones. Is there some reason this can't just hash the Vec<TreeAndJoint>? It also doesn't hash IsJoint, which seems questionable.
There was a problem hiding this comment.
@petrochenkov Do you know why this ignores IsJoint?
|
I tried converting more |
cjgillot
force-pushed
the
hashstable
branch
2 times, most recently
from
031a359
to
782cc9f
Compare
Zoxc
reviewed
Nov 25, 2019
| @@ -18,7 +18,12 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; | |||
|
|
|||
| impl<'ctx> rustc_target::StableHashingContextLike for StableHashingContext<'ctx> {} | |||
|
|
|||
| impl_stable_hash_for!(struct ::syntax::ast::Lifetime { id, ident }); | |||
| impl<'a> HashStable<StableHashingContext<'a>> for ast::Lifetime { | |||
There was a problem hiding this comment.
This could use HashStable_Generic now?
There was a problem hiding this comment.
It requires hashing the NodeId. I have a commit with this change, I can add it to this PR.
There was a problem hiding this comment.
Ah, ok. You can leave that for a later PR.
|
I guess you'd have to change all the impls to be generic. That doesn't really seem like a win. I guess we can just keep |
|
@bors r+ |
|
📌 Commit 782cc9f has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 25, 2019
Use proc-macro to derive HashStable everywhere Hello, A second proc-macro is added to derive HashStable for crates librustc depends on. This proc-macro `HashStable_Generic` (to bikeshed) allows to decouple code and strip much of librustc's boilerplate. Still, two implementations `Span` and `TokenKind` require to be placed in librustc. The latter only depends on the `bug` macro. Advise welcome on how to sever that link. A trait `StableHasingContextLike` has been introduced at each crate root, in order to handle those implementations which require librustc's very `StableHashingContext`. This overall effort allowed to remove the `impl_stable_hash_for` macro. Each commit passes the `x.py check`. I still have to double check there was no change in the implementation.
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello,
A second proc-macro is added to derive HashStable for crates librustc depends on.
This proc-macro
HashStable_Generic(to bikeshed) allows to decouple code and strip much of librustc's boilerplate.Still, two implementations
SpanandTokenKindrequire to be placed in librustc.The latter only depends on the
bugmacro. Advise welcome on how to sever that link.A trait
StableHasingContextLikehas been introduced at each crate root,in order to handle those implementations which require librustc's very
StableHashingContext.This overall effort allowed to remove the
impl_stable_hash_formacro.Each commit passes the
x.py check.I still have to double check there was no change in the implementation.