Repository navigation
Conversation
|
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. |
|
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
left a comment
There was a problem hiding this comment.
Good start! Left some suggestions (in addition to running ./x.py test src/test/ui --bless to fix the reference files).
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)
There was a problem hiding this comment.
I'd prefer to keep the constant, using the macro extension I suggested in the main PR.
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 :)
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
There was a problem hiding this comment.
yeah, I think we should make this u32 still...
There was a problem hiding this comment.
...and cast to usize here, just the once
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. |
|
@bors r+ -- nice! |
|
📌 Commit 862d25c has been approved by |
…atsakis Declare DebruijnIndex via newtype_index macro Part of #49887 Declare `DebruijnIndex` via the `newtype_index` macro.
|
☀️ Test successful - status-appveyor, status-travis |
Part of #49887
Declare
DebruijnIndexvia thenewtype_indexmacro.