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.

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

Update intravisit docs #93815

Merged
merged 1 commit into from
May 1, 2022
Merged

Update intravisit docs #93815

merged 1 commit into from
May 1, 2022

Conversation

camsteffen
Copy link
Contributor

Follow-up to #90986.

r? @cjgillot

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 9, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2022
cjgillot
cjgillot reviewed Feb 9, 2022
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks @camsteffen! Just a few nits.

compiler/rustc_hir/src/intravisit.rs Outdated
/// fix it appropriately.
/// `visit_nested_XXX` methods. If a new `visit_nested_XXX` variant is
/// added in the future, we will see the panic in your code (from
/// `nested_visit_map`) and fix it appropriately.
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is you and who is we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know the question is rhetorical but...
you="well-meaning rustc contributer with much to learn" and we="the experts who keep the wheels turning"

compiler/rustc_hir/src/intravisit.rs Outdated
/// `nested_visit_map` or use the "shallow" or "deep" visit
/// Invoked when a nested item is encountered. By default does nothing
/// unless you override `Self::NestedFilter` to a type other than
/// `nested_filter::None`, in which case it will walk the item. **You
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say: "By default, when Self::NestedFilter is nested_filter::None, this method does nothing.

compiler/rustc_hir/src/intravisit.rs Outdated
/// `nested_visit_map` to return other than `None`, in which case it will walk
/// the body.
/// `visit_nested_item`, does nothing by default unless you override
/// `Self::NestedFilter` to a type other than `None`, in which case
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `Self::NestedFilter` to a type other than `None`, in which case
/// `Self::NestedFilter`.

and stop the doc here. The description on visit_nested_itemshould be enough.

compiler/rustc_hir/src/itemlikevisit.rs Outdated
/// - How: Implement `intravisit::Visitor` and override the `nested_visit_map()` method
/// to return `NestedVisitorMap::OnlyBodies` and use
/// - How: Implement `intravisit::Visitor` and override the `NestedFilter` type to
/// `nested_filter::OnlyBodies` (and implement `nested_visit_map`), and use
/// `tcx.hir().krate().visit_all_item_likes(&mut visitor.as_deep_visitor())`. Within
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `tcx.hir().krate().visit_all_item_likes(&mut visitor.as_deep_visitor())`. Within
/// `tcx.hir().visit_all_item_likes(&mut visitor.as_deep_visitor())`. Within

@camsteffen
Copy link
Contributor Author

Addressed nits. Will squash if all is well.

cjgillot
cjgillot reviewed Feb 11, 2022
View reviewed changes
compiler/rustc_hir/src/itemlikevisit.rs Outdated
/// invoked on `tcx.hir().krate()`.
/// - How: Implement `intravisit::Visitor` and override the `NestedFilter` type to
/// `nested_filter::All` (and implement `nested_visit_map`). Walk your crate with
/// `intravisit::walk_crate()` invoked on `tcx.hir().krate()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

walk_crate does not exist any more. The user should call tcx.hir().walk_toplevel_module(visitor).

@cjgillot
Copy link
Contributor

Thanks! r=me with one nit + squashed.

@bors
Copy link
Contributor

bors commented Feb 12, 2022

☔ The latest upstream changes (presumably #93921) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2022
@JohnCSimon
Copy link
Member

@camsteffen

Ping from triage: Can you please post the status of this PR?

@camsteffen
Copy link
Contributor Author

This should be ready now but, since some time has passed, I'll let @cjgillot review once more in case anything has changed.

@camsteffen
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2022
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 30, 2022

📌 Commit 61ba32b has been approved by cjgillot

@bors 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 Apr 30, 2022
@bors
Copy link
Contributor

bors commented Apr 30, 2022

⌛ Testing commit 61ba32b with merge 2c858a7...

@bors
Copy link
Contributor

bors commented May 1, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 2c858a7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 1, 2022
@bors bors merged commit 2c858a7 into rust-lang:master May 1, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 1, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2c858a7): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels
merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
7 participants
@camsteffen @cjgillot @bors @JohnCSimon @rust-timer @rust-highfive @rustbot