-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test for eval order for a+=b #79293
Merged
Merged
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
Havvy
added
the
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
label
Nov 22, 2020
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
label
Nov 22, 2020
Yes, the order of evaluation *does* change depending on the types of the operands. Cursed, I know. I've elected to place this test into `expr/compound-assignment` creating both the `expr` directory and the `compound-assignment` directory. I plan in a future PR to also move the `if` directory and the loose `if` tests into `expr/if` and other similar cleanups of the `test/ui` directory. Future work: Test more than just `+=`, but all operators. I don't know if using a macro to generate these tests cases would be okay or not, but it'd be boilerplatey without it. I'm also confident you cannot change the evaluation order of one operator without changing all of them. Future work: Additionally, test more than just `i32 += i32` for the primitive version. I don't actually know the full set of primitive implementations, but I imagine there's enough to cause a combinatorial explosion with the previous future work item. Somewhere on the order of one to two hundred individual functions.
Havvy
force-pushed
the
test-eval-order-compound-assign
branch
from
November 22, 2020 11:13
bc3ef0d
to
b6f9705
Compare
It seems good to add a test for this -- I suspect there's probably at least one already out there, due to NLL/MIR work, but I wasn't able to quickly find it so this seems fine. @bors r+ rollup |
📌 Commit b6f9705 has been approved by |
bors
added
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Nov 22, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 22, 2020
Rollup of 10 pull requests Successful merges: - rust-lang#76941 (Add f{32,64}::is_subnormal) - rust-lang#77697 (Split each iterator adapter and source into individual modules) - rust-lang#78305 (Stabilize alloc::Layout const functions) - rust-lang#78608 (Stabilize refcell_take) - rust-lang#78793 (Clean up `StructuralEq` docs) - rust-lang#79267 (BTreeMap: address namespace conflicts) - rust-lang#79293 (Add test for eval order for a+=b) - rust-lang#79295 (BTreeMap: fix minor testing mistakes in rust-lang#78903) - rust-lang#79297 (BTreeMap: swap the names of NodeRef::new and Root::new_leaf) - rust-lang#79299 (Stabilise `then`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
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.
Yes, the order of evaluation does change depending on the types of
the operands. Cursed, I know.
I've elected to place this test into
expr/compound-assignment
creatingboth the
expr
directory and thecompound-assignment
directory. Iplan in a future PR to also move the
if
directory and the looseif
tests into
expr/if
and other similar cleanups of thetest/ui
directory.
Future work: Test more than just
+=
, but all operators. I don't knowif using a macro to generate these tests cases would be okay or not,
but it'd be boilerplatey without it. I'm also confident you cannot
change the evaluation order of one operator without changing all of
them.
Future work: Additionally, test more than just
i32 += i32
for theprimitive version. I don't actually know the full set of primitive
implementations, but I imagine there's enough to cause a combinatorial
explosion with the previous future work item. Somewhere on the order of
one to two hundred individual functions.