Skip to content
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 regression tests to ensure stable drop order #43125

Merged
merged 1 commit into from
Jul 14, 2017
Merged

Add regression tests to ensure stable drop order #43125

merged 1 commit into from
Jul 14, 2017

Conversation

aochagavia
Copy link
Contributor

@aochagavia aochagavia commented Jul 8, 2017

Work towards #43034

I think this is all we need to do on the testing front regarding RFC 1857

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

assert_eq!(*dropped_fields.borrow(), &[1, 2, 3]);

// Panic during struct construction means that fields are treated as local variables
// Therefore they are dropped in reverse order of declaration
Copy link
Contributor

@TimNN TimNN Jul 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not entirely correct. As far as I know, here the fields are dropped in reverse order of initialization (not declaration) -- that is, If you swapped the lines with x: and y: below, x would be dropped before y. I think it would be good to have tests, for structs and enum-struct variants, which use different initialization and declaration orders.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right... I always confuse these terms... I am going to correct the comment and add more tests.

@aochagavia
Copy link
Contributor Author

Just pushed two tests more to ensure different field initialization order behaves as expected

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jul 11, 2017

I would consider merging this with the mir-drop-order.rs test, but it mostly tests drop glue/destructors rather than "MIR" drop order, so we can keep them separate. Maybe rename it to rfc1857-drop-order.rs so we can find it easily? r+ modulo that.

@aturon aturon assigned arielb1 and unassigned aturon Jul 12, 2017
@aochagavia
Copy link
Contributor Author

@arielb1 done!

@arielb1
Copy link
Contributor

arielb1 commented Jul 12, 2017

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 12, 2017

📌 Commit f86e433 has been approved by arielb1

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Jul 13, 2017
Add regression tests to ensure stable drop order

Work towards rust-lang#43034

I think this is all we need to do on the testing front regarding RFC 1857
bors added a commit that referenced this pull request Jul 14, 2017
Rollup of 7 pull requests

- Successful merges: #42926, #43125, #43157, #43167, #43187, #43203, #43204
- Failed merges:
@bors bors merged commit f86e433 into rust-lang:master Jul 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants