Move ast::Item::ident into ast::ItemKind
#138740
Conversation
`global_allocator_spans` and `alloc_error_handler_span` are identical except for `name`.
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…try> Move `ast::Item::ident` into `ast::ItemKind` r? `@ghost`
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (960f748): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.7%, secondary -1.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary -0.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 773.468s -> 775.556s (0.27%) |
rustbot
added
perf-regression
|
Performance is basically neutral. If anything, the improvements slightly outweigh the regressions, but all changes are very small. @rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
Instead of putting the item inside it, just pass the ident and visibility (the only things needed) alongside it where necessary. This helps with the next commit, which will move the ident's location. Specifically, it gets rid of the `match visitor_kind` in `rewrite_type_alias`.
`FmtVisitor::visit_mac` has an `Option<Ident>` arg which is always either `None` or `Some(kw::Empty)`, because `ItemKind::MacCall` always has an empty ident. This value is passed through various functions until it reaches `rewrite_macro_name`, which treats `None` and `Some(kw::Empty)` the same. In other words, the argument is useless. This commit removes it. There is no change in behaviour. The commit also changes a few `symbol::Ident` occurrences to `Ident` in `macros.rs`; `Symbol` is imported in that file so `Ident` might as well be, too. (This is a good example of why it's a bad idea for `Itemt` to have an `ident` field when various item kinds don't have an identifier. It's easy to get confused when "empty identifier" is used to mean "no identifier". This will be fixed in a subsequent commit.)
`expand_test_case` looks for any item with a `#[test_case]` attribute and adds a `test_path_symbol` attribute to it while also fiddling with the item's ident's span. This is pretty weird, because `#[test_case]` is only valid on `fn`/`const`/`static` items, as far as I can tell. But you don't currently get an error or warning if you use it on other kinds of items. This commit changes things so that a `#[test_case]` item is modified only if it is `fn`/`const`/`static`. This is relevant for moving idents from `Item` to `ItemKind`, because some item kinds don't have an ident, e.g. `impl` blocks. The commit also does the following. - Renames a local variable `test_id` as `test_ident`. - Changes a `const` to `static` in `tests/ui/custom_test_frameworks/full.rs` to give the `static` case some test coverage. - Adds a `struct` and `impl` to the same test to give some test coverage to the non-affected item kinds. These have a `FIXME` comment identifying the weirdness here. Hopefully this will be useful breadcrumbs for somebody else in the future.
`ast::Item` has an `ident` field. - It's always non-empty for these item kinds: `ExternCrate`, `Static`, `Const`, `Fn`, `Mod`, `TyAlias`, `Enum`, `Struct`, `Union`, `Trait`, `TraitAlias`, `MacroDef`, `Delegation`. - It's always empty for these item kinds: `Use`, `ForeignMod`, `GlobalAsm`, `Impl`, `MacCall`, `DelegationMac`. There is a similar story for `AssocItemKind` and `ForeignItemKind`. Some sites that handle items check for an empty ident, some don't. This is a very C-like way of doing things, but this is Rust, we have sum types, we can do this properly and never forget to check for the exceptional case and never YOLO possibly empty identifiers (or possibly dummy spans) around and hope that things will work out. The commit is large but it's mostly obvious plumbing work. Some notable things. - `ast::Item` got 8 bytes bigger. This could be avoided by boxing the fields within some of the `ast::ItemKind` variants (specifically: `Struct`, `Union`, `Enum`). I might do that in a follow-up; this commit is big enough already. - For the visitors: `FnKind` no longer needs an `ident` field because the `Fn` within how has one. - In the parser, the `ItemInfo` typedef is no longer needed. It was used in various places to return an `Ident` alongside an `ItemKind`, but now the `Ident` (if present) is within the `ItemKind`. - In a few places I renamed identifier variables called `name` (or `foo_name`) as `ident` (or `foo_ident`), to better match the type, and because `name` is normally used for `Symbol`s. It's confusing to see something like `foo_name.name`.
nnethercote
force-pushed
the
ast-ItemKind-idents
branch
from
87677af to
c977a9d
Compare
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
@fmease: thanks for reviewing, and apologies for the size of the final commit. I can move the four preliminary commits into a separate PR if that would be helpful. |
The follow-up to #138384, which did the same thing for
hir::ItemKind.r? @fmease