Skip to content

Tidy: disallow TODO in other in-tree projects#153166

Merged
rust-bors[bot] merged 3 commits intorust-lang:mainfrom
reddevilmidzy:codegen-tidy
Mar 15, 2026
Merged

Tidy: disallow TODO in other in-tree projects#153166
rust-bors[bot] merged 3 commits intorust-lang:mainfrom
reddevilmidzy:codegen-tidy

Conversation

@reddevilmidzy
Copy link
Member

@reddevilmidzy reddevilmidzy commented Feb 27, 2026

@rustbot rustbot added A-tidy Area: The tidy tool S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 27, 2026
@reddevilmidzy reddevilmidzy marked this pull request as ready for review February 27, 2026 06:21
@rustbot
Copy link
Collaborator

rustbot commented Feb 27, 2026

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2026
@bjorn3
Copy link
Member

bjorn3 commented Feb 27, 2026

Tidy does not run inside the cg_clif and cg_gcc repos, which means whenever we do a sync, there is a fair chance that we did be forced to male changes when tidy complains.

@jieyouxu
Copy link
Member

jieyouxu commented Feb 27, 2026

Tidy does not run inside the cg_clif and cg_gcc repos, which means whenever we do a sync, there is a fair chance that we did be forced to male changes when tidy complains.

I would suggest not running full tidy against the cg_* subtrees yeah.

Also, doesn't rust-lang/compiler-team#963 only say TODO -> FIXME? Not other tidy checks? I do not think we should be enforcing other tidy rules for the cg_* subtrees.

@reddevilmidzy
Copy link
Member Author

Ah, I overinterpreted it — my mistake. Thanks for the clarification. I’ll update it accordingly.

@reddevilmidzy reddevilmidzy marked this pull request as draft February 27, 2026 10:58
@rustbot rustbot 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 Feb 27, 2026
@rust-bors

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the codegen-tidy branch 2 times, most recently from 89a1cb9 to 2617d54 Compare March 7, 2026 12:39
@reddevilmidzy reddevilmidzy changed the title Allow tidy check in codegen Tidy: disallow TODO in other in-tree projects Mar 7, 2026
Copy link
Member Author

@reddevilmidzy reddevilmidzy left a comment

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to add cg_* checks to style.rs, which checks TODOs in tidy, but since it also checks other things besides TODOs, I separated it into codegen.rs.

}
}

// todo: this function now accepts `Session` instead of `ParseSess` and should be relocated
Copy link
Member Author

Choose a reason for hiding this comment

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

lowercase 👀

@reddevilmidzy reddevilmidzy marked this pull request as ready for review March 7, 2026 12:50
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2026
@rust-log-analyzer

This comment has been minimized.

.enumerate()
.map(|(index, field)| {
self.context.new_field(None, *field, format!("field{}_TODO", index))
self.context.new_field(None, *field, format!("field{}_FIXME", index))
Copy link
Contributor

Choose a reason for hiding this comment

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

is that fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I'll check this again. Thank you

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed TODO to FIXME and tested it and it worked.

rust-lang/rustc_codegen_gcc@d82d242

cc: @antoyo

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this should be OK.

let is_codegen_tidy_file = file.ends_with(codegen_file);
if is_codegen_tidy_file {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why this?

Copy link
Member Author

Choose a reason for hiding this comment

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

a035848#diff-2877cff958f18d248fbdb0e5f924a453e3911e7e8340139125911d3bdd8e5a62R49-R53

I'm using the "TODO" character here and top line, this is the part that was added to skip TODO check

Copy link
Contributor

Choose a reason for hiding this comment

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

we already do it for this_file separately, can you the checks for these two?

after that r=me

@reddevilmidzy
Copy link
Member Author

thank you for the review
@bors r=lcnr

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 13, 2026

@reddevilmidzy: 🔑 Insufficient privileges: not in review users

@reddevilmidzy
Copy link
Member Author

@lcnr It's not working because I don't have permission... haha, Please roll it up.

@ShoyuVanilla
Copy link
Member

Though I'm not a compiler team member, I have the permission and it would be okay for me to add this to queue as lcnr said the following 😅

after that r=me

@bors r=lcnr

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 15, 2026

📌 Commit e09936e has been approved by lcnr

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2026
@rust-bors

This comment has been minimized.

@rust-bors rust-bors bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 15, 2026
@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 15, 2026

☀️ Test successful - CI
Approved by: lcnr
Duration: 3h 13m 55s
Pushing 1e21831 to main...

@rust-bors rust-bors bot merged commit 1e21831 into rust-lang:main Mar 15, 2026
12 checks passed
@rustbot rustbot added this to the 1.96.0 milestone Mar 15, 2026
@github-actions
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing d27207d (parent) -> 1e21831 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 1e2183119f0ee19cc26df899e26b04ad0de3475d --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. aarch64-apple: 2h 54m -> 2h 22m (-18.5%)
  2. aarch64-gnu: 2h 1m -> 2h 23m (+18.1%)
  3. dist-x86_64-apple: 2h -> 1h 41m (-15.9%)
  4. x86_64-gnu-gcc: 1h 13m -> 1h 1m (-15.6%)
  5. x86_64-msvc-1: 2h 43m -> 2h 20m (-14.1%)
  6. dist-ohos-armv7: 1h 15m -> 1h 8m (-9.2%)
  7. x86_64-mingw-2: 2h 38m -> 2h 25m (-8.3%)
  8. dist-x86_64-illumos: 1h 47m -> 1h 38m (-8.1%)
  9. dist-aarch64-apple: 1h 56m -> 1h 47m (-7.9%)
  10. aarch64-gnu-llvm-21-1: 1h 2m -> 57m 18s (-7.7%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1e21831): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.9% [0.1%, 1.8%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -3.5%, secondary -2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.5% [-3.5%, -3.5%] 1
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) -3.5% [-3.5%, -3.5%] 1

Cycles

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

Binary size

Results (secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.0% [-0.0%, -0.0%] 1
All ❌✅ (primary) - - 0

Bootstrap: 481.726s -> 481.243s (-0.10%)
Artifact size: 394.80 MiB -> 394.84 MiB (0.01%)

@reddevilmidzy reddevilmidzy deleted the codegen-tidy branch March 16, 2026 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-tidy Area: The tidy tool merged-by-bors This PR was explicitly merged by bors. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

Tracking issue for MCP 963: Extend the x.py policy for TODO and FIXME to other in-tree projects

9 participants