Skip to content

Replace span_look_ahead with span_followed_by#154745

Open
chenyukang wants to merge 1 commit intorust-lang:mainfrom
chenyukang:yukang-fix-span-api
Open

Replace span_look_ahead with span_followed_by#154745
chenyukang wants to merge 1 commit intorust-lang:mainfrom
chenyukang:yukang-fix-span-api

Conversation

@chenyukang
Copy link
Copy Markdown
Member

@chenyukang chenyukang commented Apr 3, 2026

While reviewing that PR #154703 (comment), I found that magic number 100, let's remove it, and seems span_followed_by is a better name.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 3, 2026

Some changes occurred in match checking

cc @Nadrieril

@rustbot rustbot added 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. labels Apr 3, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 3, 2026

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 69 candidates
  • Random selection from 11 candidates

// In case this could be a struct literal that needs to be surrounded
// by parentheses, find the appropriate span.
let close_brace_span = sm.span_look_ahead(followed_brace_span, "}", Some(50));
let close_brace_span =
Copy link
Copy Markdown
Member Author

@chenyukang chenyukang Apr 3, 2026

Choose a reason for hiding this comment

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

we can not use span_look_ahead here, because we only want to make sure there is a } in 50 range.

otherwise we can not handle the scenario like U { /* keep comment here */ }, since there are some non whitespace chars inside { }.

https://github.com/rust-lang/rust/pull/154745/changes#diff-68dd28577bfdbdba4a4c4ee9d20797f2f0e3939d926d4bdcbe13b1117a2dbe32R35-R38 is the test case for this.

Copy link
Copy Markdown
Member

@Kivooeo Kivooeo left a comment

Choose a reason for hiding this comment

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

it looks reasonable to me, but I'd like someone else to take a look too

View changes since this review

let close_brace_span =
sm.span_to_next_source(open_brace_span).ok().and_then(|next_source| {
let offset = next_source.find('}')?;
if next_source[..offset].chars().count() >= 50 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining what this >= 50 check is doing, and why 50 was chosen.

@nnethercote nnethercote 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 Apr 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants