ast: Generalize item kind visiting #124382
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
| @@ -34,6 +34,10 @@ impl<A: Array> ExpectOne<A> for SmallVec<A> { | |||
| } | |||
| } | |||
|
|
|||
| pub trait NoopVisitItemKind { | |||
There was a problem hiding this comment.
using noop here is confusing to me. This seems to match SuperVisitable for types, so I'd really like this to be renamed to sth like trait SuperVisitableItem
There was a problem hiding this comment.
I used the naming convention that is used everywhere else in mut_visit.rs, there are dozens of noop_visits there.
(Same applies to visit.rs.)
There was a problem hiding this comment.
yeah, i dislike that 😢 i guess it's fine to keep it, but if you have the time, I would be really happy to see it changed in a separate PR then
There was a problem hiding this comment.
I think it's better to keep the consistent naming, and at some point change the naming scheme in one go (if change at all).
(I don't have any hard feelings towards the current naming personally, it's been around for like 10 years, and at least everyone working with AST knows it.)
There was a problem hiding this comment.
sure, I would like to see it changed in one go. Anyways, for this PR then please just do
| pub trait NoopVisitItemKind { | |
| pub trait NoopVisitableItem { |
There was a problem hiding this comment.
The style guide recommends against "able" in traits though - https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#trait-naming (at least that's what standard library follows, so I've been always following it in the compiler as well).
There was a problem hiding this comment.
(Also, this is about ItemKind specifically, not Item)
There was a problem hiding this comment.
I guess that's alright 👍 we do use -able for quite a few existing traits, esp the existing ty visitors, but 🤷
| @@ -34,6 +34,10 @@ impl<A: Array> ExpectOne<A> for SmallVec<A> { | |||
| } | |||
| } | |||
|
|
|||
| pub trait NoopVisitItemKind { | |||
| fn noop_visit(&mut self, visitor: &mut impl MutVisitor); | |||
There was a problem hiding this comment.
same here
| fn noop_visit(&mut self, visitor: &mut impl MutVisitor); | |
| fn super_visit(mut self, visitor: &mut impl MutVisitor); |
| @@ -102,6 +102,15 @@ pub enum LifetimeCtxt { | |||
| GenericArg, | |||
| } | |||
|
|
|||
| pub trait WalkItemKind: Sized { | |||
There was a problem hiding this comment.
| pub trait WalkItemKind: Sized { | |
| pub trait WalkableKind: Sized { |
And avoid duplicating logic for visiting `Item`s with different kinds (regular, associated, foreign).
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
ast: Generalize item kind visiting And avoid duplicating logic for visiting `Item`s with different kinds (regular, associated, foreign). The diff is better viewed with whitespace ignored.
Rollup of 3 pull requests Successful merges: - rust-lang#124370 (Fix substitution parts having a shifted underline in some cases) - rust-lang#124382 (ast: Generalize item kind visiting) - rust-lang#124387 (thread_local: be excruciatingly explicit in dtor code) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#124382 (ast: Generalize item kind visiting) - rust-lang#124387 (thread_local: be excruciatingly explicit in dtor code) - rust-lang#124427 (Add missing tests for an ICE) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#124382 - petrochenkov:itemvisit, r=lcnr ast: Generalize item kind visiting And avoid duplicating logic for visiting `Item`s with different kinds (regular, associated, foreign). The diff is better viewed with whitespace ignored.
And avoid duplicating logic for visiting
Items with different kinds (regular, associated, foreign).The diff is better viewed with whitespace ignored.