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

Make trans treat all statements as if they introduce a new scope #4192

Closed
wants to merge 1 commit into from

Conversation

catamorphism
Copy link
Contributor

r? @graydon This is for consistency with borrowck. Changing borrowck would
also be an alternative, but I found it easier to change trans :-)

This eliminates an ICE in trans where the scope for a particular
borrow was a statement ID, but the code in trans that does cleanups
wasn't finding the block with that scope. As per #3860

This is for consistency with borrowck. Changing borrowck would
also be an alternative, but I found it easier to change trans :-)

This eliminates an ICE in trans where the scope for a particular
borrow was a statement ID, but the code in trans that does cleanups
wasn't finding the block with that scope. As per rust-lang#3860
@graydon
Copy link
Contributor

graydon commented Dec 15, 2012

r+ with a couple caveats:

  • it shouldn't cause anything else to break: I'm assuming this scope isn't going to accidentally cause stuff to be freed early or something? A trans scope isn't exactly a {..} lifetime-of-a-variable thing, right?
  • check that it's not a huge perf regression to be adding all these scopes. perf stat before-and-after runs to see.
  • enhance the comment in trans_stmt here so it points to region::resolve_stmt, and add a comment there pointing back to trans::base::trans_stmt, so that both sites indicate their inter-relatedness.

Sorry, this stuff is delicate-city and I know we're likely to revisit it. Best to lay down invariant-describing docs as we go.

@catamorphism
Copy link
Contributor Author

  1. No, because this is always adding a larger scope as a super-scope of a scope that would have existed before. If that's clear.
  2. I'll try that
  3. Ditto

@catamorphism
Copy link
Contributor Author

Instruments.app reported a total 13m:30s build time for a clean build of make perf with the incoming branch, and 15m:30s with this change. (With optimization enabled.) That seems pretty bad, so I'll think about other ways to accomplish this.

@catamorphism
Copy link
Contributor Author

I'm going to close this since it's pretty clearly not the right solution. The bug is still open.

flip1995 pushed a commit to flip1995/rust that referenced this pull request May 15, 2020
…flip1995

Reversed empty ranges

This lint checks range expressions with inverted limits which result in empty ranges. This includes also the ranges used to index slices.

The lint reverse_range_loop was covering iteration of reversed ranges in a for loop, which is a subset of what this new lint covers, so it has been removed. I'm not sure if that's the best choice. It would be doable to check in the new lint that we are not in the arguments of a for loop; I went for removing it because the logic was too similar to keep them separated.

changelog: Added reversed_empty_ranges lint that checks for ranges where the limits have been inverted, resulting in empty ranges. Removed reverse_range_loop which was covering a subset of the new lint.

Closes rust-lang#4192
Closes rust-lang#96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants