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

Use clang-format in tidy to check the C++ code style under llvm-wrapper #123918

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

DianQK
Copy link
Member

@DianQK DianQK commented Apr 14, 2024

@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 14, 2024
@rust-log-analyzer

This comment has been minimized.

@Dylan-DPC Dylan-DPC added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2024
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 4, 2024
@DianQK DianQK changed the title [test] clang-format [WIP] clang-format Jun 4, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 16, 2024

☔ The latest upstream changes (presumably #126540) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 16, 2024
@rust-log-analyzer

This comment has been minimized.

@DianQK DianQK changed the title [WIP] clang-format Use clang-format in tidy to check the C++ code style under llvm-wrapper Jun 25, 2024
@DianQK
Copy link
Member Author

DianQK commented Jun 25, 2024

@rustbot ready
@rustbot label -S-experimental

@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. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Jun 25, 2024
@DianQK DianQK marked this pull request as ready for review June 25, 2024 14:28
@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@DianQK
Copy link
Member Author

DianQK commented Jun 26, 2024

The job mingw-check-tidy failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

This is the unformatted actual output.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 26, 2024

Looks great, thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 26, 2024

📌 Commit c163d5c has been approved by Kobzol

It is now in the queue for this repository.

@bors bors 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 Jun 26, 2024
@bors
Copy link
Contributor

bors commented Jun 27, 2024

⌛ Testing commit c163d5c with merge 7033f9b...

@bors
Copy link
Contributor

bors commented Jun 27, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 7033f9b to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7033f9b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary -3.8%)

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.

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

Cycles

Results (secondary -2.0%)

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.

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

Binary size

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

Bootstrap: 696.035s -> 694.579s (-0.21%)
Artifact size: 326.75 MiB -> 326.68 MiB (-0.02%)

@DianQK DianQK deleted the clang-format branch June 27, 2024 04:49
@nikic
Copy link
Contributor

nikic commented Jul 9, 2024

What is the command to call this locally?

I tried ./x.py test tidy -- tidyselftest --extra-checks=cpp:fmt (which is probably not the usual way to spell it?) but it did not report formatting errors that CI does.

@DianQK
Copy link
Member Author

DianQK commented Jul 9, 2024

What is the command to call this locally?

I tried ./x.py test tidy -- tidyselftest --extra-checks=cpp:fmt (which is probably not the usual way to spell it?) but it did not report formatting errors that CI does.

It's ./x test --stage 0 src/tools/tidy tidyselftest --extra-checks=cpp:fmt.

@nikic
Copy link
Contributor

nikic commented Jul 9, 2024

I see... It seems like the minimal command to make this work is ./x.py test tidy --extra-checks=cpp:fmt.

What confused me is that ./x.py tidy --extra-checks=cpp:fmt does not work (and suggests using -- --extra-checks=cpp:fmt instead, which does not work either).

@nikic
Copy link
Contributor

nikic commented Jul 9, 2024

I'm also wondering why TIDY_PRINT_DIFF=1 is not enabled by default. The default output seems to be unhelpful to the point of uselessness.

@DianQK
Copy link
Member Author

DianQK commented Jul 9, 2024

What confused me is that ./x.py tidy --extra-checks=cpp:fmt does not work (and suggests using -- --extra-checks=cpp:fmt instead, which does not work either).

I guess this is unexpected. It should print:

error: 'x.py' requires a subcommand but one was not provided
 [subcommands: build, b, check, c, clippy, fix, fmt, doc, d, test, t, miri, bench, clean, dist, install, run, r, setup, suggest, vendor, perf]

I'm also wondering why TIDY_PRINT_DIFF=1 is not enabled by default. The default output seems to be unhelpful to the point of uselessness.

I guess it's because we can use --bless to fix the formatting problem and lint doesn't have diff to show.

cc @Kobzol

@Kobzol
Copy link
Contributor

Kobzol commented Jul 9, 2024

I agree that displaying the diff by default is a good idea (not sure why we didn't do it before). When you run x test tidy and some Rust code is unformatted, it also prints the diff by default, even without any special flags.

@tgross35 Any objections against just printing the Python/C++ linting/formatting diff by default?

@tgross35
Copy link
Contributor

tgross35 commented Jul 9, 2024

That sounds good to me, I don't know why I didn't just do that in the first place.

I have also been meaning to change

/// comma-separated list of other files types to check (accepts py, py:lint,
/// py:fmt, shell)
extra_checks: Option<String>,
to a Vec<ext_tool_checks::Arg> + enum Arg { Py, PyLint, Cpp, ... } so Clap handles printing the available options, since that doc comment is out of date. I'll probably get to that this week but if somebody beats me to it, be my guest 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .clang-format file
9 participants