Uh oh!
There was an error while loading. Please reload this page.
Tidy CFTE/MIRI - #53609
Merged
Merged
Conversation
rust-highfive
commented
Aug 22, 2018
Contributor
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
commented
Aug 22, 2018
Contributor
|
lovesegfault
commented
Aug 22, 2018
ContributorAuthor
Ah crap, I accidentally added those submodule changes. Fixing. |
varkor
reviewed
Aug 22, 2018
Contributor
There was a problem hiding this comment.
Leave a newline after the header (here and elsewhere).
lovesegfaultforce-pushed
the
tidy-ctfe
branch
2 times, most recently
from
August 22, 2018 17:52
ba0695d to
27b8c92Comparerust-highfive
commented
Aug 22, 2018
Contributor
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 |
RalfJung
commented
Aug 22, 2018
Member
r? @RalfJung |
rust-highfive
commented
Aug 22, 2018
Contributor
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 |
Member
There was a problem hiding this comment.
This needs a backslash to not change the actual string being computed:
"attempted to do invalid arithmetic on pointers that would leak base addresses, \
e.g. comparing pointers into different allocations",
Here and elsewhere.
Member
There was a problem hiding this comment.
This removes the space.
"a\
b"
is the string "ab". See playground.
You should add a space in front of the backslash.
Member
Please compile-test code locally before pushing it. :)
|
lovesegfault
commented
Aug 22, 2018
ContributorAuthor
@RalfJung Fixed everything & rebased. |
lovesegfault
commented
Aug 22, 2018
ContributorAuthor
Oh ffs the submodule changes made it through |
rust-highfive
commented
Aug 22, 2018
Contributor
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 |
bors
commented
Aug 23, 2018
Collaborator
☔ The latest upstream changes (presumably #52011) made this pull request unmergeable. Please resolve the merge conflicts. |
Member
Please do not use merging to resolve conflicts; always do a rebase. And sorry that you are experiencing all these conflicts, that is often a problem with PRs touching so many files :/ I will try to get it reviewed ASAP once you fixed them. |
Member
There was a problem hiding this comment.
Our usual way of formatting function signatures multi-line is
fn cast_from_float(&self,bits:u128,fty:FloatTy,dest_ty:Ty<'tcx>) -> EvalResult<'tcx,Scalar>{Please follow that.
Member
There was a problem hiding this comment.
Same for the multi-line function signature style.
Member
There was a problem hiding this comment.
After a multi-line conditional, please put the { on its own line.
RalfJung
commented
Aug 23, 2018
Member
This is the official style for non-inherent impls: so I'd say having indented where clauses is in the same spirit. Also, CI found two lines that are still too long. |
| bits: u128, | ||
| fty: FloatTy, | ||
| dest_ty: Ty<'tcx> | ||
| ) -> EvalResult<'tcx, Scalar> { |
Member
There was a problem hiding this comment.
The line with the arrow should not be indented:
fn cast_from_float(
&self,
bits: u128,
fty: FloatTy,
dest_ty: Ty<'tcx>
) -> EvalResult<'tcx, Scalar> {
// code goes here
That nicely groups the arguments.
lovesegfault
commented
Aug 23, 2018
ContributorAuthor
@RalfJung Fixed :) |
| &self, | ||
| def_id: DefId, | ||
| substs: &'tcx Substs<'tcx> | ||
| ) -> EvalResult<'tcx, ty::Instance<'tcx>> { |
| pub fn read_scalar(&self, ptr: Pointer, ptr_align: Align, size: Size) -> EvalResult<'tcx, ScalarMaybeUndef> { | ||
| self.check_relocation_edges(ptr, size)?; // Make sure we don't read part of a pointer as a pointer | ||
| pub fn read_scalar(&self, ptr: Pointer, ptr_align: Align, size: Size) | ||
| -> EvalResult<'tcx, ScalarMaybeUndef> { |
Member
There was a problem hiding this comment.
And another one (oops I think I had missed that before as well)
| pub fn unpack_unsized_mplace( | ||
| &self, | ||
| mplace: MPlaceTy<'tcx> | ||
| ) -> EvalResult<'tcx, MPlaceTy<'tcx>> { |
RalfJung
commented
Aug 23, 2018
Member
I think those are the last three... sorry I hadn't seen them earlier.^^ |
lovesegfault
commented
Aug 23, 2018
ContributorAuthor
@RalfJung I think I got them all now 😄 . Thanks! |
RalfJung
commented
Aug 23, 2018
Member
:) Let us see if CI is happy. |
lovesegfault
commented
Aug 23, 2018
ContributorAuthor
@RalfJung SHIP IT |
RalfJung
commented
Aug 23, 2018
Member
I will :) @bors r+ |
bors
commented
Aug 23, 2018
Collaborator
📌 Commit e07c154 has been approved by |
lovesegfault
commented
Aug 23, 2018
ContributorAuthor
@RalfJung Cheers! Thanks for walking me through my first PR! |
RalfJung
commented
Aug 23, 2018
Member
Thanks for contributing :) |
This was referenced Aug 24, 2018
Merged
bors
commented
Aug 25, 2018
Collaborator
bors
commented
Aug 25, 2018
Collaborator
☀️ Test successful - status-appveyor, status-travis |
bors added a commit
that referenced
this pull request
Aug 26, 2018
Miri engine cleanup * Unify the two maps in memory to store the allocation and its kind together. * Share the handling of statics between CTFE and miri: The miri engine always uses "lazy" `AllocType::Static` when encountering a static. Acessing that static invokes CTFE (no matter the machine). The machine only has any influence when writing to a static, which CTFE outright rejects (but miri makes a copy-on-write). * Add an `AllocId` to by-ref consts so miri can use them as operands without making copies. * Move responsibilities around for the `eval_fn_call` machine hook: The hook just has to find the MIR (or entirely take care of everything); pushing the new stack frame is taken care of by the miri engine. * Expose the intrinsics and lang items implemented by CTFE so miri does not have to reimplement them. * Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks). * Clean up function calling. * Switch const sanity check to work on operands, not mplaces. * Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details. In particular, we can finally make `eval_operand` take `&self`. :-) Should be merged after #53609, across which I will rebase.
bors added a commit
that referenced
this pull request
Aug 27, 2018
Miri engine cleanup * Unify the two maps in memory to store the allocation and its kind together. * Share the handling of statics between CTFE and miri: The miri engine always uses "lazy" `AllocType::Static` when encountering a static. Acessing that static invokes CTFE (no matter the machine). The machine only has any influence when writing to a static, which CTFE outright rejects (but miri makes a copy-on-write). * Add an `AllocId` to by-ref consts so miri can use them as operands without making copies. * Move responsibilities around for the `eval_fn_call` machine hook: The hook just has to find the MIR (or entirely take care of everything); pushing the new stack frame is taken care of by the miri engine. * Expose the intrinsics and lang items implemented by CTFE so miri does not have to reimplement them. * Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks). * Clean up function calling. * Switch const sanity check to work on operands, not mplaces. * Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details. In particular, we can finally make `eval_operand` take `&self`. :-) Should be merged after #53609, across which I will rebase.
bors added a commit
that referenced
this pull request
Aug 29, 2018
Miri engine cleanup * Unify the two maps in memory to store the allocation and its kind together. * Share the handling of statics between CTFE and miri: The miri engine always uses "lazy" `AllocType::Static` when encountering a static. Acessing that static invokes CTFE (no matter the machine). The machine only has any influence when writing to a static, which CTFE outright rejects (but miri makes a copy-on-write). * Add an `AllocId` to by-ref consts so miri can use them as operands without making copies. * Move responsibilities around for the `eval_fn_call` machine hook: The hook just has to find the MIR (or entirely take care of everything); pushing the new stack frame is taken care of by the miri engine. * Expose the intrinsics and lang items implemented by CTFE so miri does not have to reimplement them. * Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks). * Clean up function calling. * Switch const sanity check to work on operands, not mplaces. * Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details. In particular, we can finally make `eval_operand` take `&self`. :-) Should be merged after #53609, across which I will rebase.
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Fixes#53596