Skip to content

Fix clang-tidy findings in src/ (#1925) - #2278

Closed
devtejasx wants to merge 1 commit into
google:mainfrom
devtejasx:clang-tidy-cleanups
Closed

devtejasx wants to merge 1 commit into
google:mainfrom
devtejasx:clang-tidy-cleanups

Conversation

@devtejasx

@devtejasx devtejasx commented Aug 1, 2026 •

Copy link
Copy Markdown
Contributor

Works through part of the collated clang-tidy list, restricted to changes that cannot alter behaviour:

  • misc-const-correctness: declare locals const where they are never reassigned, spelling the qualifier first to match the surrounding code.
  • readability-uppercase-literal-suffix: uppercase integer literal suffixes.
  • readability-simplify-boolean-expr: distribute the negation in IsTruthyFlagValue() instead of negating a disjunction, and drop a manual counting loop in favour of the value it was recomputing.
  • misc-use-anonymous-namespace: move the file-local console reporter helpers IgnoreColorPrint() and FormatTime() into an anonymous namespace rather than marking them static.

No functional change. Release and debug builds are clean and the test suite passes.


Per AGENTS.md: AI-assisted — the patch was drafted with AI assistance
(Claude) and then reviewed, tested, and understood by me. I take full
responsibility for it.

@dmah42

dmah42 commented Aug 3, 2026

Copy link
Copy Markdown
Member

did you consider why these checks aren't enabled?

Three findings from the checks the project's own .clang-tidy already globs
in, limited to changes that cannot alter behaviour:

- readability-uppercase-literal-suffix: `0u` -> `0U`.
- readability-simplify-boolean-expr: distribute the negation over the
  disjunction in RunInThread() and in IsTruthyFlagValue(), rather than
  negating the whole expression.
- misc-use-anonymous-namespace: the file-local console reporter helpers
  IgnoreColorPrint() and FormatTime() move into an anonymous namespace
  instead of being marked static.

This deliberately leaves out misc-const-correctness, which accounts for most
of what clang-tidy reports here and is the part that is purely a matter of
taste. Happy to add or drop any of the above.

No functional change. Release and debug builds are clean and the test suite
passes.
@devtejasx
devtejasx force-pushed the clang-tidy-cleanups branch from ed9e8d9 to ebf6a31 Compare August 3, 2026 09:55
@devtejasx

Copy link
Copy Markdown
Contributor Author

I read the question as: the config globs them in, but that is not the same as the project wanting them applied. .clang-tidy does list misc-* and readability-*, and clang-tidy 20 reports six misc-const-correctness hits in complexity.cc alone — but WarningsAsErrors is empty and nothing runs it in CI, so the glob is a starting point for #1925 rather than a mandate.

So I have cut this down to the three findings that are not a matter of taste:

  • 0u to 0U,
  • two negated disjunctions distributed (RunInThread() and IsTruthyFlagValue()),
  • the two file-local console reporter helpers moved from static into an anonymous namespace.

The misc-const-correctness batch, which was most of the previous diff, is gone. If what is left is still more churn than you want against #1925, say so and I'll close it.

@dmah42

dmah42 commented Aug 3, 2026

Copy link
Copy Markdown
Member

so maybe we should start by running clang-tidy on CI (or even on pre-commit) before we add more fixes. fwiw, i wouldn't care about u vs U. the anonymous namespace fix is nice.

@devtejasx

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback. It sounds like agreeing on clang tidy enforcement should come first, so I will close this PR for now. Once the project decides which checks to enforce, I can send focused cleanup patches that match that polic

@devtejasx devtejasx closed this Aug 3, 2026
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.

2 participants