Issue 49938: Reference tagged unions discr(iminant) as tag #50309
+18
−18
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
|
Can you rebase over the current master branch? some changes seem to have happened to the code you touched. You can reference the issue with |
|
r? @oli-obk |
shepmaster
added
the
S-waiting-on-review
|
Sorry for the delay. It was late and I valued sleep more than fun with git rebase. I've rebased on I see travis-CI just failed. I'm going to need help with this one. Looking at that log, I don't know how to overcome this problem. |
|
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 |
|
No worries. Take all the time you need. The rebase went wrong. I'm not sure what exactly, but there's a good chance another rebase will fix it. Try |
|
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 |
|
Still failing. I tried to rebase again with I'll have to revisit this tomorrow again. |
oli-obk
reviewed
Apr 30, 2018
src/librustc/ty/layout.rs
Outdated
| @@ -149,6 +149,183 @@ pub const FAT_PTR_ADDR: usize = 0; | |||
| /// - For a slice, this is the length. | |||
| pub const FAT_PTR_EXTRA: usize = 1; | |||
|
|
|||
| /// Describes how the fields of a type are located in memory. | |||
There was a problem hiding this comment.
You can remove this entire block of "new" code
src/librustc_mir/hair/pattern/mod.rs
Outdated
| @@ -837,7 +837,36 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { | |||
| ConstVal::Value(val) => { | |||
| let variant_index = const_variant_index( | |||
| self.tcx, self.param_env, instance, val, cv.ty | |||
| ).expect("const_variant_index failed"); | |||
| ).expect("const_discr failed"); | |||
There was a problem hiding this comment.
same here, just use the old code
|
☔ The latest upstream changes (presumably #50345) made this pull request unmergeable. Please resolve the merge conflicts. |
samWson
force-pushed
the
issue-49938
branch
2 times, most recently
from
f90fc3f to
e9593ea
Compare
|
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 |
|
Still seem to be failing. @oli-obk i followed your directions. Then I No success. |
|
Sorry. I meant |
|
☔ The latest upstream changes (presumably #50198) made this pull request unmergeable. Please resolve the merge conflicts. |
samWson
force-pushed
the
issue-49938
branch
2 times, most recently
from
f9798ba to
e8b473d
Compare
|
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 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 |
|
After |
oli-obk
reviewed
May 2, 2018
src/librustc_mir/hair/pattern/mod.rs
Outdated
| @@ -838,6 +838,35 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { | |||
| let variant_index = const_variant_index( | |||
| self.tcx, self.param_env, instance, val, cv.ty | |||
| ).expect("const_variant_index failed"); | |||
| let layout = self | |||
src/librustc/ty/layout.rs
Outdated
| @@ -148,7 +151,183 @@ pub const FAT_PTR_ADDR: usize = 0; | |||
| /// - For a slice, this is the length. | |||
| pub const FAT_PTR_EXTRA: usize = 1; | |||
|
|
|||
| #[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable)] | |||
| #[derive(PartialEq, Eq, Hash, Debug)] | |||
There was a problem hiding this comment.
You ca delete all the green code here
src/librustc/ty/layout.rs
Outdated
| @@ -20,6 +20,9 @@ use std::fmt; | |||
| use std::i128; | |||
| use std::mem; | |||
|
|
|||
| use std::iter; | |||
There was a problem hiding this comment.
This can probably be removed, too
|
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 |
samWson
commented
May 4, 2018
| Tagged { | ||
| discr: Scalar, | ||
| tag: Scalar, | ||
| variants: Vec<LayoutDetails>, |
There was a problem hiding this comment.
I'm not sure if this declaration of Tagged here is describing the in memory representation of a tagged union, but my code wouldn't compile because of something I changed in layout.rs I think. So I made the change as shown here.
There was a problem hiding this comment.
That's fine, this enum seems to have been moved from librustc to here
|
Green at last! Fixes for #49938 ready. Please review and let me know if any more changes are required. |
eddyb
reviewed
May 5, 2018
src/librustc/ty/layout.rs
Outdated
| @@ -1057,7 +1057,7 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> { | |||
| } | |||
| tcx.intern_layout(LayoutDetails { | |||
| variants: Variants::Tagged { | |||
| discr: tag, | |||
| tag: tag, | |||
eddyb
reviewed
May 5, 2018
src/librustc/ty/layout.rs
Outdated
| // Discriminant field for enums (where applicable). | ||
| Variants::Tagged { ref discr, .. } | | ||
| Variants::NicheFilling { niche: ref discr, .. } => { | ||
| // Tag field for enums (where applicable). |
There was a problem hiding this comment.
This is still "Discriminant" (which is either a tag or a niche), I would change ref discr to tag: ref discr.
Refer rust-lang#49938 Previously tagged unions' tag was refered to as a discr(iminant). Here the changes use tag instead which is the correct terminology when refering to the memory representation of tagged unions.
|
@eddyb changes have been made and latest build is green. Please review and comment. |
|
@bors r+ Thanks! |
|
📌 Commit 38a6eca has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 38a6eca with merge 50d5a0264a922c8c663688e1ce948b3636a9aad4... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
@bors retry
|
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
May 6, 2018
Issue 49938: Reference tagged unions discr(iminant) as tag Here the changes reference the Tagged type _discriminant_ as _tag_ instead. This is the correct terminology when referencing how tagged unions are represented in memory.
|
☀️ 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.
Here the changes reference the Tagged type discriminant as tag instead. This is the correct terminology when referencing how tagged unions are represented in memory.