Skip to content

Should all the AST node types with attributes be using AttrVec? #77662

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

Closed
Ryan1729 opened this issue Oct 7, 2020 · 0 comments · Fixed by #86385
Closed

Should all the AST node types with attributes be using AttrVec? #77662

Ryan1729 opened this issue Oct 7, 2020 · 0 comments · Fixed by #86385
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-parser Area: The lexing & parsing of Rust source code to an AST C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Ryan1729
Copy link
Contributor

Ryan1729 commented Oct 7, 2020

I've been working on a PR to clippy that involved looking at the different AST node types in rustc_ast::ast. I noticed that while most of the subset of those types that can contain attributes use AttrVec, (an alias of a ThinVec of Attributes) some node types use a plain Vec<Attribute>.

In particular, as of this writing, Variant, StructField, Item, Crate, and Arm all use Vec<Attribute>. (I found these via a search in rustc_ast for "attr" and checking each field in the results, so hopefully that list is complete.)

ThinVec's documentation says:

A vector type optimized for cases where this size is usually 0 (cf. SmallVector). The Option<Box<..>> wrapping allows us to represent a zero sized vector with None, which uses only a single (null) pointer.

I would suspect that for most of the types that use a plain Vec<Attribute> the Vec is empty almost all of the time, with the possible exception of Items, since #[derive(Debug)] etc. is pretty common.

So should more of the types that use a plain Vec<Attribute> be using an AttrVec instead? Are these fields plain Vecs just because no one changed them, or was it somehow determined that those types aren't copied enough for this optimization to matter?

@jonas-schievink jonas-schievink added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-parser Area: The lexing & parsing of Rust source code to an AST C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 7, 2020
@bors bors closed this as completed in 1a46283 Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-parser Area: The lexing & parsing of Rust source code to an AST C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants