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.

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

Remove the span from ast::Lit. #101516

Closed

Conversation

nnethercote
Copy link
Contributor

In the most common case, the Lit is stored within an Expr, which records the same span.

For other cases, we can store the span next to the Lit.

r? @ghost

In the most common case, the `Lit` is stored within an `Expr`, which
records the same span.

For other cases, we can store the span next to the `Lit`.
@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 7, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2022

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 7, 2022
@bors
Copy link
Contributor

bors commented Sep 7, 2022

⌛ Trying commit 6d478fd with merge 38d57e8411697109f017e5190da471ee297dbd52...

@bors
Copy link
Contributor

bors commented Sep 7, 2022

☀️ Try build successful - checks-actions
Build commit: 38d57e8411697109f017e5190da471ee297dbd52 (38d57e8411697109f017e5190da471ee297dbd52)

@rust-timer
Copy link
Collaborator

Queued 38d57e8411697109f017e5190da471ee297dbd52 with parent 8c41305, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (38d57e8411697109f017e5190da471ee297dbd52): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking 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.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-3.2%, -2.0%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-3.2%, -2.0%] 2

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 7, 2022
@calebcartwright
Copy link
Member

calebcartwright commented Sep 7, 2022

I probably won't have a chance to look at this for a few days, but just wanted to note I'd like a chance to take a closer look at potential rustfmt impacts before this gets merged if possible.

In the most common case, the Lit is stored within an Expr, which records the same span.

This sounds fair to me, it's just out of an abundance of caution from the rustfmt perspective that I want to dive in because people always seem to put comments in weird places, and rustfmt relies heavily on precise spans in the AST to avoid dropping (or butchering) comments, and unfortunately we don't always have robust tests for such cases

@petrochenkov petrochenkov self-assigned this Sep 7, 2022
@nnethercote
Copy link
Contributor Author

No problem. I'm pretty sure I won't have broken everything because I
double-checked that places where expression literals are constructed were
already using the literal's span as the expression's span. E.g. in the
following functions:

  • Parser::parse_literal_maybe_minus
    • This is the simplest case, where the old code was let expr = self.mk_expr(lit.span, ExprKind::Lit(lit));.
  • MetaItemKind::mac_args
  • ExtCtxt::expr_lit

The one counterexample is Parser::parse_bottom_expr, which was using
lo.to(self.prev_token.span) instead of literal.span. But I put in a
temporary assertion that those two spans are equal and all the rustc and
rustfmt tests passed, and the new code uses the literal span directly instead
of lo.to(self.prev_token.span).

@nnethercote
Copy link
Contributor Author

I have moved this into #101562, where it's one of several changes required to shrink ast::Expr.

@nnethercote nnethercote closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants