Skip to content

refactor TreeVisitor #4710

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.

Sign up for GitHub

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

Jump to bottom
Merged
RalfJung merged 1 commit into from
Nov 21, 2025
Merged

refactor TreeVisitor #4710

RalfJung merged 1 commit into from
Nov 21, 2025
+170 −230

Conversation

@royAmmerschuber
Copy link
Contributor

@royAmmerschuber royAmmerschuber commented Nov 18, 2025

Refactors tree borrows TreeVisitor with the following changes:

  • Changing it to take UniIndex instead of tag as start point
  • Remove the error handling callback. It's not really necessary now that we have access to the entire tree in the propagate callback.

@rustbot
Copy link
Collaborator

rustbot commented Nov 18, 2025

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Nov 18, 2025
RalfJung
RalfJung requested changes Nov 19, 2025
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Thanks!

However, it looks like you may have to be a bit more careful with your rebases.

View changes since this review

src/borrow_tracker/tree_borrows/tree.rs Outdated
stack.finish_foreign_accesses(&mut self)
}

/// Like `traverse_this_parents_children_other`, but skips the children of `start`.
Copy link
Member

@RalfJung RalfJung Nov 19, 2025

Choose a reason for hiding this comment

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

The doc comment still refers to start, which you renamed.

The same occurs in traverse_this_parents_children_other.

@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Nov 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 19, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Nov 19, 2025
RalfJung
RalfJung reviewed Nov 19, 2025
View reviewed changes
@royAmmerschuber
Copy link
Contributor Author

royAmmerschuber commented Nov 19, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 19, 2025
@RalfJung
Copy link
Member

RalfJung commented Nov 19, 2025

This looks great, thanks! Please squash the commits. Then write @rustbot ready after you force-pushed the squashed PR.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Nov 19, 2025
@royAmmerschuber
Copy link
Contributor Author

royAmmerschuber commented Nov 20, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 20, 2025
@RalfJung RalfJung enabled auto-merge November 20, 2025 20:55
@RalfJung
Copy link
Member

RalfJung commented Nov 20, 2025

Great, thanks!

Once this lands, please rebase your other PR.

RalfJung
RalfJung approved these changes Nov 21, 2025
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

I keep forgetting I have to do this, so annoying...

View changes since this review

@RalfJung RalfJung added this pull request to the merge queue Nov 21, 2025
Merged via the queue into rust-lang:master with commit 5fbfc88 Nov 21, 2025
13 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@royAmmerschuber @rustbot @RalfJung