Declare DebruijnIndex via newtype_index macro #51248
Merged
+50
−56
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
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
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 |
|
EDIT: @nikomatsakis had a better suggestion below, ignore me. |
|
@fabric-and-ink two things:
Lines 447 to 451 in ae2f299 |
nikomatsakis
requested changes
May 31, 2018
| @@ -131,10 +131,10 @@ impl Region { | |||
| fn shifted(self, amount: u32) -> Region { | |||
There was a problem hiding this comment.
I think we could change this from amount: u32 to amount: usize and avoid the casts below.
There was a problem hiding this comment.
(but see my comment below about maybe changing shifted_in instead)
| @@ -1861,7 +1861,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { | |||
| .map(|(i, input)| { | |||
| let mut gather = GatherLifetimes { | |||
| map: self.map, | |||
| outer_index: ty::DebruijnIndex::INNERMOST, | |||
| outer_index: ty::DebruijnIndex::innermost(), | |||
There was a problem hiding this comment.
I'd prefer to keep the constant, using the macro extension I suggested in the main PR.
src/librustc/ty/fold.rs
Outdated
| /// | | here, would be INNTERMOST shifted in by 1 | ||
| /// | here, initially, binder would be INNERMOST | ||
| /// | | here, would be innermost shifted in by 1 | ||
| /// | here, initially, binder would be innermost |
There was a problem hiding this comment.
can prob change these back to CAPITALS, but it looks like the original had a typo (INNTERMOST) though, we should fix that :)
src/librustc/ty/fold.rs
Outdated
| @@ -565,7 +565,7 @@ impl<'a, 'gcx, 'tcx> TypeFolder<'gcx, 'tcx> for RegionReplacer<'a, 'gcx, 'tcx> { | |||
| pub fn shift_region(region: ty::RegionKind, amount: u32) -> ty::RegionKind { | |||
| match region { | |||
| ty::ReLateBound(debruijn, br) => { | |||
| ty::ReLateBound(debruijn.shifted_in(amount), br) | |||
| ty::ReLateBound(debruijn.shifted_in(amount as usize), br) | |||
There was a problem hiding this comment.
hmm, instead of changing all of these to make amount: usize — which we could do — perhaps we should just change shifted_in to take a u32 instead
src/librustc/ty/sty.rs
Outdated
| @@ -1272,31 +1269,31 @@ impl DebruijnIndex { | |||
| /// | |||
| /// you would need to shift the index for `'a` into 1 new binder. | |||
| #[must_use] | |||
| pub const fn shifted_in(self, amount: u32) -> DebruijnIndex { | |||
| DebruijnIndex { index: self.index + amount } | |||
| pub fn shifted_in(self, amount: usize) -> DebruijnIndex { | |||
There was a problem hiding this comment.
yeah, I think we should make this u32 still...
src/librustc/ty/sty.rs
Outdated
| pub const fn shifted_in(self, amount: u32) -> DebruijnIndex { | ||
| DebruijnIndex { index: self.index + amount } | ||
| pub fn shifted_in(self, amount: usize) -> DebruijnIndex { | ||
| DebruijnIndex::new(self.index() + amount) |
There was a problem hiding this comment.
...and cast to usize here, just the once
nikomatsakis
added
S-waiting-on-author
We should use that more in MIR, sorry about my own suggestion 😄. |
|
Thanks for the feedback! I try to work it in as soon as I can. (Quite possibly tomorrow.) |
|
@fabric-and-ink ok lemme know :) |
|
Got it!
|
|
Dang, made a typo. Hang on... |
|
Ok, should be fine now. Just one thing seemed strange: |
|
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 |
|
Ok, it seems that the I suppose that |
|
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 |
|
@fabric-and-ink You can avoid using Also, I don't think having |
|
I can leave I don't know why the current build fails. The type is a tuple struct, isn't it? But why the error message? |
|
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 |
|
Ah now I get why the error is happening. Well, I think the |
|
@fabric-and-ink I'd rather keep using the macro, just so that everything is consolidated, and you can use const D2: DebuijnIndex = DebruijnIndex::INNERMOST.shifted_in(1); |
|
Ok, I think I got it now. |
nikomatsakis
approved these changes
Jun 18, 2018
nikomatsakis
approved these changes
Jun 18, 2018
|
@bors r+ -- nice! |
|
📌 Commit 862d25c has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jun 18, 2018
…atsakis Declare DebruijnIndex via newtype_index macro Part of #49887 Declare `DebruijnIndex` via the `newtype_index` macro.
|
☀️ Test successful - status-appveyor, status-travis |
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.
Part of #49887
Declare
DebruijnIndexvia thenewtype_indexmacro.