optimize reassignment immutable state#53258
Merged
bors merged 4 commits intorust-lang:masterfrom Aug 19, 2018
Merged
Conversation
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
|
@bors try |
Collaborator
|
⌛ Trying commit 898e603d6aa573b9c689907efae4d3e418598842 with merge bbb5fcca30e1090333a8200c3aa3304777570f26... |
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 |
Contributor
Author
|
@bors try- |
nikomatsakis
force-pushed
the
issue-53189-optimize-reassignment-immutable-state
branch
from
898e603 to
2982be6
Compare
Contributor
Author
|
@bors try |
Collaborator
|
⌛ Trying commit 2982be69d72789ae400e7100a29c17948986886f with merge 1cb82c2cd55f855bf84a37a9c20236fab3e9c685... |
nikomatsakis
added
S-waiting-on-perf
Collaborator
|
☀️ Test successful - status-travis |
Member
|
@rust-timer build 1cb82c2cd55f855bf84a37a9c20236fab3e9c685 |
Collaborator
|
Success: Queued 1cb82c2cd55f855bf84a37a9c20236fab3e9c685 with parent 0aa8d03, comparison URL. |
Contributor
|
Perf results are in; looking good, especially for keccak. |
nikomatsakis
added
S-waiting-on-review
Contributor
Author
|
I was a bit disturbed since no tests broke when I changed this code. So I added some tests for various interesting scenarios (cc #21232). |
pnkfelix
reviewed
Aug 12, 2018
Contributor
There was a problem hiding this comment.
it might be good to promote these tests outside of the nll/ directory (and get rid of #![feature(nll)]), and let the compare-mode=nll encode the differences in behavior between NLL and AST-borrowck. Just a thought.
Contributor
There was a problem hiding this comment.
(clearly I didn't care enough about whether this comment was addressed to let it hold up my r+. Still, feel free to incorporate this change if you can get to it before bors merges this.)
Contributor
|
@bors r+ |
Collaborator
|
📌 Commit a3457ae68a971eef1a402ad232796a22b6e71bee has been approved by |
Collaborator
|
🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
Contributor
Author
|
@bors r=pnkfelix |
Collaborator
|
📌 Commit 6c21c02124c3be45a959eb128a2756dd171f970b has been approved by |
Collaborator
|
⌛ Testing commit 6c21c02124c3be45a959eb128a2756dd171f970b with merge e7097535de4b74eb7f7bb224e69748c48b9d4b5e... |
Collaborator
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
Member
|
UI test failures. ui\immut-function-arguments.rs#mir ---- [ui] ui\immut-function-arguments.rs#mir stdout ----
diff of stderr:
- error[E0384]: cannot assign to immutable argument `y`
+ error[E0594]: cannot assign to `*y`, as `y` is not declared as mutable
2 --> $DIR/immut-function-arguments.rs:15:5
3 |
4 LL | fn f(y: Box<isize>) {
- | - consider changing this to `mut y`
+ | - help: consider changing this to be mutable: `mut y`
6 LL | *y = 5; //[ast]~ ERROR cannot assign
- | ^^^^^^ cannot assign to immutable argument
+ | ^^^^^^ cannot assign
8
- error[E0384]: cannot assign to immutable argument `q`
+ error[E0594]: cannot assign to `*q`, as `q` is not declared as mutable
10 --> $DIR/immut-function-arguments.rs:20:35
11 |
12 LL | let _frob = |q: Box<isize>| { *q = 2; }; //[ast]~ ERROR cannot assign
- | - ^^^^^^ cannot assign to immutable argument
+ | - ^^^^^^ cannot assign
14 | |
- | consider changing this to `mut q`
+ | help: consider changing this to be mutable: `mut q`
16
17 error: aborting due to 2 previous errors
18
- For more information about this error, try `rustc --explain E0384`.
+ For more information about this error, try `rustc --explain E0594`.
20 ui\mut\mutable-class-fields.rs#mir ---- [ui] ui\mut\mutable-class-fields.rs#mir stdout ----
diff of stderr:
- error[E0384]: cannot assign twice to immutable variable `nyan`
+ error[E0594]: cannot assign to `nyan.how_hungry`, as `nyan` is not declared as mutable
2 --> $DIR/mutable-class-fields.rs:28:3
3 |
4 LL | let nyan : cat = cat(52, 99);
- | ----
- | |
- | first assignment to `nyan`
- | consider changing this to `mut nyan`
+ | ---- help: consider changing this to be mutable: `mut nyan`
9 LL | nyan.how_hungry = 0; //[ast]~ ERROR cannot assign
- | ^^^^^^^^^^^^^^^^^^^ cannot assign twice to immutable variable
+ | ^^^^^^^^^^^^^^^^^^^ cannot assign
11
12 error: aborting due to previous error
13
- For more information about this error, try `rustc --explain E0384`.
+ For more information about this error, try `rustc --explain E0594`.
15 |
kennytm
added
S-waiting-on-author
Don't iterate over all things that are initialized.
We did not seem to have any!
now compare-mode can show us the differences
nikomatsakis
force-pushed
the
issue-53189-optimize-reassignment-immutable-state
branch
from
6c21c02 to
58e4b54
Compare
Contributor
Author
|
@bors r=pnkfelix |
Collaborator
|
📌 Commit 58e4b54 has been approved by |
bors
added
S-waiting-on-bors
Collaborator
bors
added a commit
that referenced
this pull request
Aug 19, 2018
…-immutable-state, r=pnkfelix optimize reassignment immutable state This is the "simple fix" when it comes to checking for reassignment. We just shoot for compatibility with the AST-based checker. Makes no attempt to solve #21232. I opted for this simpler fix because I didn't want to think about complications [like the ones described here](#21232 (comment)). Let's do some profiling measurements. Fixes #53189 r? @pnkfelix
Collaborator
|
☀️ Test successful - status-appveyor, status-travis |
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 free
to 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.
This is the "simple fix" when it comes to checking for reassignment. We just shoot for compatibility with the AST-based checker. Makes no attempt to solve #21232.
I opted for this simpler fix because I didn't want to think about complications like the ones described here.
Let's do some profiling measurements.
Fixes #53189
r? @pnkfelix