Promoteds are statics and statics have a place, not just a value #52597
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
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 |
nagisa
reviewed
Jul 22, 2018
| promoted: Some(index), | ||
| }; | ||
| let c = bx.tcx().const_eval(param_env.and(cid)); | ||
| let (llval, ty) = self.simd_shuffle_indices( |
There was a problem hiding this comment.
SIMD shuffle indices are an array, which usually is specified inline, and not via an explicit constant. so the array is promoted and we need to read it here
There was a problem hiding this comment.
I see. A comment to that effect would be great.
nagisa
reviewed
Jul 22, 2018
src/librustc_mir/borrow_check/mod.rs
Outdated
| @@ -2030,6 +2039,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | |||
| }), | |||
| } | |||
| } | |||
| // promoteds may never be mutated | |||
| Place::Promoted(_) => Err(place), | |||
There was a problem hiding this comment.
This should probably be a bug!()? I can’t think of any case where you’d genuinely want to report an error to the user if you somehow end up with a &mut promoted at this point in code.
nagisa
reviewed
Jul 22, 2018
| @@ -326,6 +327,9 @@ fn place_element_conflict<'a, 'gcx: 'tcx, 'tcx>( | |||
| Overlap::EqualOrDisjoint | |||
| } | |||
| } | |||
| (Place::Promoted(_), Place::Promoted(_)) | | |||
There was a problem hiding this comment.
Shouldn’t this also return an EqualOrDisjoint if the two promoteds are the same?
|
This is r=me once #52571 lands and the issues are fixed. |
|
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 |
oli-obk
force-pushed
the
promotion_simplify
branch
from
c0ab624
to
d4ab267
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 latest upstream changes (presumably #52571) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
force-pushed
the
promotion_simplify
branch
from
d3f05e5
to
cbd4274
Compare
RalfJung
reviewed
Jul 24, 2018
| "could not evaluate constant operand", | ||
| ); | ||
| // Allow RalfJ to sleep soundly knowing that even refactorings that remove | ||
| // the above error (or silence it under some conditions) will not cause UB |
|
@bors r=nagisa |
|
📌 Commit cbd4274 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 24, 2018
Promoteds are statics and statics have a place, not just a value r? @eddyb This makes everything around promoteds a little simpler
|
☀️ Test successful - status-appveyor, status-travis |
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Jul 24, 2018
Tested on commit rust-lang/rust@6a3db03. Direct link to PR: <rust-lang/rust#52597> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
|
Merged
eddyb
reviewed
Aug 2, 2018
| // and we can then extract the value by evaluating the promoted. | ||
| mir::Operand::Copy(mir::Place::Promoted(box(index, ty))) | | ||
| mir::Operand::Move(mir::Place::Promoted(box(index, ty))) => { | ||
| let param_env = ty::ParamEnv::reveal_all(); |
There was a problem hiding this comment.
This should probably be something like self.param_env (minor future hazard if we start deduplicating instances).
There was a problem hiding this comment.
The PR already landed.Does anyone take care of fixing this?^^
There was a problem hiding this comment.
This is rustc_codegen_llvm... we don't have a param env, everything is monomorphized
eddyb
reviewed
Aug 2, 2018
| let type_checker = &mut self.cx; | ||
|
|
||
| // FIXME -- For now, use the substitutions from | ||
| // `value.ty` rather than `value.val`. The |
There was a problem hiding this comment.
Heh, value.val mention here is outdated (cc @nikomatsakis).
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.
r? @eddyb
This makes everything around promoteds a little simpler