-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Implement a simple diagnostic system for tidy #146592
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
Conversation
The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
in theory, this kinda adds to a general concern i have: i think however, i think this specific case is probably fine, at least conceptually. |
Yeah, I agree with that (although that's orthogonal to this PR). We already have a system for passing |
(the verbosity reduction was implemented in #146609) |
(I think the verbosity could still be tweaked a bit, like with all the git commands fetching info, but that's still orthogonal to this PR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, changes look good to me. Very glad to see some more systematic treatment of tidy's diagnostics logic.
Can you please squash the commits (since some of them are intermediary and some don't quite correspond to the commit messages as-is)?
You can r=me after.
@rustbot author |
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@bors r=jieyouxu |
☀️ Test successful - checks-actions |
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 1d23da6 (parent) -> 6710835 (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 6710835ae739ca326441ff6c63d24fb123858300 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (6710835): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -8.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 5.0%, secondary -2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 470.725s -> 470.892s (0.04%) |
In #146316 and #146580, contributors independently wanted to reduce the verbose output of tidy. But before, the output was quite ad-hoc, so it was not easy to control it.
In this PR, I implemented a simple diagnostic system for tidy, which allows us to:
-v
)println
s left, but most output should be scoped to a specific check)Failure output:

Success output:

Verbose output (shortened):

CC @nnethercote @RalfJung @GuillaumeGomez
The first two commits and the last commit are interesting, the rest is just mechanical port of the code from
bad: &mut bool
toDiagCtx
andRunningCheck
.The
extra_checks
check could be further split, but I'd leave that for another PR.r? @jieyouxu