C++ linting via clang-tidy #1338

fandreuz · 2025-06-12T13:21:31Z

Description

In this PR I add some lint checks based on clang-tidy. They are mostly aimed at reducing time spent on trivial problems in code review, which can easily be automated.

Checks

The checks I enabled required minimal changes in the code and are not too invasive.

Some examples:

naming of local variables like_this
naming of private variables _like_this
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unhandled-self-assignment.html
some performance issues:

/workspaces/async-profiler/src/index.h:21:29: error: the parameter 'value' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
    u32 indexOf(std::string value) {
                            ^
                const      &
/workspaces/async-profiler/src/index.h:29:65: error: the parameter 'consumer' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
    void forEachOrdered(std::function<void(const std::string&)> consumer) const {
                                                                ^
                        const                                  &

some common mistakes:

/workspaces/async-profiler/src/threadLocalData.cpp:33:60: error: the comparison always evaluates to false because pthread_setspecific always returns non-negative values [bugprone-posix-return,-warnings-as-errors]
    if (pthread_setspecific(profiler_data_key, (void*)val) < 0) {
                                                           ^
                                                           >

Full list of available linter checks: https://clang.llvm.org/extra/clang-tidy/checks/list.html

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

.clang-tidy

src/frameName.cpp

src/rustDemangle.cpp

This reverts commit f604191.

src/ctimer_linux.cpp

Makefile

src/ctimer_linux.cpp

Makefile

CONTRIBUTING.md

Makefile

docker/code-check.Dockerfile

apangin · 2025-07-01T00:21:13Z

docker/code-check.Dockerfile

+
+ADD https://raw.githubusercontent.com/llvm/llvm-project/refs/tags/llvmorg-20.1.7/clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py /usr/bin/
+RUN apk add --no-cache clang-extra-tools linux-headers make python3 git && \
+    chmod +x /usr/bin/clang-tidy-diff.py


I see ADD has --chmod option, does it work here?

Had no idea it existed, thanks: a6cb304

So you no longer need to run chmod +x, right?

Right: 32d9bb3

apangin · 2025-07-01T00:23:19Z

docker/code-check.Dockerfile

+# Image for all tasks related to static code analysis in Async-Profiler
+FROM public.ecr.aws/docker/library/amazoncorretto:11-alpine-jdk
+
+ADD https://raw.githubusercontent.com/llvm/llvm-project/refs/tags/llvmorg-20.1.7/clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py /usr/bin/


That is better. However, tags are still mutable. Specific commit id would be ideal.

Sure: a6cb304

But on a side not, maybe mutability is not a bad thing for tags? If a tag was overwritten, maybe there was a bug in the release. And a commit wouldn't protect for that.

Mutability is not bad for tags, but my intention is to minimize dependencies outside our control, i.e., to mitigate supply chain threats when a third party can unilaterally change code that is run on our servers.

docker/code-check.Dockerfile

Disable JFR OldObjectSample event in jfrsync mode (async-profiler#1350) Fix invalid alignment in mallocTracer and zero-init buf in getTotalCpuTime (async-profiler#1351) C++ linting via clang-tidy (async-profiler#1338) Allow cstack=vmx for native applications (async-profiler#1354) Correctly unwind stack for malloc events in VM stack walking mode (async-profiler#1357) Simplify location handling in OTLP (async-profiler#1361) Suppress javac warnings when compiling tests Do not include excess files in test.jar Fix nonjava test failure on Alpine Auto-generated clang-tidy review comments (async-profiler#1360) JFR to OTLP converter (async-profiler#1336) Cancel redundant in-progress GHA runs (async-profiler#1363) Ensure that only files under `src/` are checked in `cpp-lint-diff` (async-profiler#1365) Publish clang-tidy comments only for non-draft PRs (async-profiler#1367) Give tests unique suffix names (async-profiler#1371) Test OTLP output format (async-profiler#1331) Disable JFR OldObjectSample event in jfrsync mode (async-profiler#1350) Fix invalid alignment in mallocTracer and zero-init buf in getTotalCpuTime (async-profiler#1351) C++ linting via clang-tidy (async-profiler#1338) Allow cstack=vmx for native applications (async-profiler#1354) Correctly unwind stack for malloc events in VM stack walking mode (async-profiler#1357) Simplify location handling in OTLP (async-profiler#1361) Suppress javac warnings when compiling tests Do not include excess files in test.jar Fix nonjava test failure on Alpine Auto-generated clang-tidy review comments (async-profiler#1360) JFR to OTLP converter (async-profiler#1336) Cancel redundant in-progress GHA runs (async-profiler#1363) Ensure that only files under `src/` are checked in `cpp-lint-diff` (async-profiler#1365) Publish clang-tidy comments only for non-draft PRs (async-profiler#1367) Give tests unique suffix names (async-profiler#1371) Test OTLP output format (async-profiler#1331)

lint stuff

c250b28

fandreuz requested a review from apangin June 12, 2025 13:21

apangin reviewed Jun 12, 2025

View reviewed changes

.clang-tidy Outdated Show resolved Hide resolved

src/frameName.cpp Outdated Show resolved Hide resolved

src/rustDemangle.cpp Outdated Show resolved Hide resolved

fandreuz added 17 commits June 12, 2025 13:50

s_

5e3d572

if space

259c11e

empty

0f0e3ea

cast not needed

011d6d1

more checks

349f264

const and explicit

f604191

nn

4d0814e

no explicit

f85c1a7

Revert "const and explicit"

7ea3072

This reverts commit f604191.

named

3e5f8c3

disable some stuff

8286407

stuff

08a2f8c

no const

aa3d235

nn

7629a5a

cpp lint

70eb80a

docker

d4dc054

fix comment

64ec77a

fandreuz marked this pull request as ready for review June 20, 2025 15:56

fandreuz added 3 commits June 20, 2025 15:58

Merge branch 'master' into cpp-lint

a222411

revert

393302f

diff

7f76c4a

apangin reviewed Jun 20, 2025

View reviewed changes

src/ctimer_linux.cpp Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

fandreuz added 5 commits June 20, 2025 20:41

ops

e760d61

no questions

b636cf9

nn

49c12dd

install make

1093bee

cc

d61dbb1

Baraa-Hasheesh reviewed Jun 23, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

src/ctimer_linux.cpp Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

fandreuz added 8 commits June 23, 2025 09:23

mkfile formatting

f711009

ops

c1d2e0f

ops

fcd5cfc

fix stuff

3b89448

clang-tidy-diff

7240ebb

cc

47a397f

no docker in makefile

8426292

contributing

c9448ac

apangin reviewed Jun 30, 2025

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

docker/code-check.Dockerfile Outdated Show resolved Hide resolved

fandreuz added 5 commits June 30, 2025 23:35

nn

4796581

no oneshell

b4fa8c5

backticks

6bcd078

use docker add

75f6beb

revert

d42d356

apangin reviewed Jul 1, 2025

View reviewed changes

fandreuz added 3 commits July 1, 2025 08:04

chmod and spelling

a6cb304

all of them

2658769

nn

32d9bb3

apangin merged commit 9b44c2e into async-profiler:master Jul 1, 2025
18 checks passed

fandreuz mentioned this pull request Jul 2, 2025

Auto-generated clang-tidy review comments #1360

Merged

visheshruparelia pushed a commit to visheshruparelia/async-profiler that referenced this pull request Jul 9, 2025

C++ linting via clang-tidy (async-profiler#1338)

6fa8d46

C++ linting via clang-tidy #1338

C++ linting via clang-tidy #1338

Uh oh!

Conversation

fandreuz commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checks

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apangin Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

fandreuz Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apangin Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

fandreuz Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

apangin Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

fandreuz Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

apangin Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fandreuz commented Jun 12, 2025 •

edited

Loading

fandreuz Jul 1, 2025 •

edited

Loading