Skip to content

chore: enable targeted bugprone clang-tidy checks#500

Open
Loyal-Young wants to merge 3 commits into
alibaba:mainfrom
Loyal-Young:chore/enable-bugprone-clang-tidy
Open

chore: enable targeted bugprone clang-tidy checks#500
Loyal-Young wants to merge 3 commits into
alibaba:mainfrom
Loyal-Young:chore/enable-bugprone-clang-tidy

Conversation

@Loyal-Young

@Loyal-Young Loyal-Young commented Jun 17, 2026

Copy link
Copy Markdown

Fixes #293.

What changed

  • Enable bugprone-multiple-statement-macro and bugprone-sizeof-expression explicitly in .clang-tidy.
  • Keep the broader bugprone-* group disabled.
  • Fix the two existing bugprone-sizeof-expression diagnostics in cube_test.cc.

Validation

  • Ran clang-tidy 18.1.8 over all 447 entries in the compilation database: no diagnostics from the enabled checks.
  • Built the cube_test target successfully.
  • Ran cube_test: 6 tests passed.
  • git diff --check

@egolearner

Copy link
Copy Markdown
Collaborator

Hi @Loyal-Young, thanks for your contribution! We'd like to enable clang-tidy and fix existing issues together in one PR — would you be able to do that? See #419 as an example.

@Loyal-Young

Copy link
Copy Markdown
Author

Hi @Loyal-Young, thanks for your contribution! We'd like to enable clang-tidy and fix existing issues together in one PR — would you be able to do that? See #419 as an example.

Thanks for the guidance. Sorry for late reply ,I had some final-exams last two weeks. I'll run the enabled bugprone checks, fix the existing violations in this PR, and update it following #419.

@Loyal-Young

Copy link
Copy Markdown
Author

Thanks for pointing me to #419. I reproduced the checks locally with clang-tidy 18.1.8.

Unlike #419, which enables a single check, enabling the full bugprone-* group currently reports approximately 1,327 diagnostics across 214 files. The largest groups are:

  • bugprone-narrowing-conversions: 666
  • bugprone-easily-swappable-parameters: 263
  • bugprone-implicit-widening-of-multiplication-result: 128

Many of these require behavior-sensitive changes rather than mechanical fixes. Would you prefer that I:

  1. keep all bugprone-* checks enabled and perform the full cleanup in this PR, or
  2. exclude the high-noise checks above and fix all remaining actionable diagnostics?

I will proceed based on your preference.

@egolearner

Copy link
Copy Markdown
Collaborator

Thanks for checking this and sharing the numbers.

Given that enabling the whole bugprone-* group currently reports ~1,327 diagnostics across 214 files, I think we should enable clang-tidy incrementally instead of doing the full cleanup in one PR. Some of the largest groups, especially bugprone-narrowing-conversions, bugprone-easily-swappable-parameters, and bugprone-implicit-widening-of-multiplication-result, need behavior-sensitive review and are likely too noisy for the first step.

For this PR, please enable a small explicit subset of bugprone-* checks with low noise and clear value, then fix all existing diagnostics from those checks. It is also fine to start with checks that produce zero or only a small number of actionable diagnostics.

Suggested direction:

  • Do not enable bugprone-* as a whole in this PR.
  • Add individual checks explicitly in .clang-tidy.
  • Prioritize checks with obvious bugs or mostly mechanical fixes, for example bugprone-use-after-move, bugprone-sizeof-expression, bugprone-macro-parentheses, bugprone-multiple-statement-macro, etc., depending on the actual local results.

We can follow up with separate PRs for the noisier checks once the first batch is in.

@Loyal-Young Loyal-Young requested a review from iaojnh as a code owner July 4, 2026 12:12
@Loyal-Young Loyal-Young changed the title chore: enable bugprone clang-tidy checks chore: enable targeted bugprone clang-tidy checks Jul 4, 2026
@Loyal-Young

Copy link
Copy Markdown
Author

Updated as suggested. The PR now enables only bugprone-multiple-statement-macro and bugprone-sizeof-expression, and all existing diagnostics from these checks have been fixed. I also ran clang-tidy across all 447 compilation database entries and verified cube_test passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clang-tidy: enable bugprone-*

4 participants