Enable clang-tidy tooling by colin-higgins · Pull Request #372 · DataDog/dd-trace-cpp

colin-higgins · 2026-09-24T14:54:54Z

Add a shared clang-tidy baseline and a CI job that fails on findings, so PRs cannot merge with tidy violations.

Co-authored-by: Cursor <cursoragent@cursor.com>

pr-commenter · 2026-09-24T15:00:56Z

Benchmarks

Benchmark execution time: 2026-09-24 17:32:53

Comparing candidate commit 70b56c4 in PR branch colin.higgins/static-analysis-groundwork with baseline commit d7a2e2a in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 8 metrics, 0 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

cataphract · 2026-09-24T15:01:59Z

I you don't pin the clang-tidy version, it's going to be very difficult to get reproductible results. What's gonna happen is that the checks are gonna pass on the dev's machine and fail on ci. This is also a problem with clang-format, but a bit less so because you can work around the problem by running clang-format directly from some docker container. This doesn't work for clang-tidy because compile_commands.json is going to have references to system headers, perhaps you're running on mac os, etc. For that, you need to run everything inside a container with cmake/compilers/etc, run cmake build dir initialization to generate compile_commands.json inside, and run clang-tidy there.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2cfeebf708

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-09-24T15:02:24Z

+# Collect first-party translation units. Headers are analyzed through them.
+# Compiler flags from the database may include -Werror. Tidy findings are the
+# gate; unknown-to-this-tidy compiler flags should not fail the job.
+find binding/ examples/ fuzz/ include/ src/ test/ \


Restrict tidy to configured translation units

The ci-clang preset enables tests and tools only; it leaves the C binding, examples, and fuzzers disabled, so their files have no entries in .build/compile_commands.json. This command still passes all of those files to clang-tidy. For example, binding/c/src/tracer.cpp is analyzed without the binding include path and fails to find datadog/c/tracer.h, causing the new verify job to fail on every PR before downstream jobs can run. Enumerate compilation-database entries instead, or enable every directory being scanned in the CI preset.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-09-24T15:02:24Z

+  readability-redundant-member-init,
+  readability-redundant-string-cstr,
+  readability-simplify-boolean-expr
+WarningsAsErrors: '*'


Make the new hard-error baseline pass

This setting turns existing diagnostics in configured translation units into CI failures. Running the configured checks against src/datadog/tracer.cpp reports readability-redundant-member-init and performance-move-const-arg, while src/datadog/remote_config/product.cpp reports two misc-static-assert diagnostics; therefore the new check cannot pass even after its input list is limited to compilation-database entries. Fix or suppress the baseline findings before making all warnings fatal.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-09-24T15:02:24Z

+  readability-redundant-string-cstr,
+  readability-simplify-boolean-expr
+WarningsAsErrors: '*'
+HeaderFilterRegex: '(src|include|binding|test|fuzz|examples)/'


Match full paths in the header filter

This pattern does not match full header paths such as include/datadog/clock.h, so clang-tidy suppresses their diagnostics rather than gating them. With the current config, clang-tidy reports the clock header's readability-redundant-member-init diagnostics only when invoked with --header-filter=.*; the normal filter emits no diagnostic and exits successfully. Prefix the directory alternatives with a full-path match (for example, .*(src|include|binding|test|fuzz|examples)/) so first-party header findings fail the check as documented.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-09-24T15:02:24Z

+# Collect first-party translation units. Headers are analyzed through them.
+# Compiler flags from the database may include -Werror. Tidy findings are the
+# gate; unknown-to-this-tidy compiler flags should not fail the job.
+find binding/ examples/ fuzz/ include/ src/ test/ \


Include configured tool sources in the tidy scan

This source-root list omits tools/, even though ci-clang enables DD_TRACE_BUILD_TOOLS and therefore creates a compilation-database entry for tools/config-inversion/main.cpp. The Development workflow builds and runs that executable after this check, but changes to its first-party C++ source will never be analyzed by clang-tidy despite the script and documentation promising coverage of first-party sources. Include tools/ here, or derive the input list from the compilation database.

Useful? React with 👍 / 👎.

Co-authored-by: Cursor <cursoragent@cursor.com>

xlamorlette-datadog

Thanks a lot for this, this will definitely help a lot!…

Please consider alternative proposed solution to run Clang Tidy “on its own”.

xlamorlette-datadog · 2026-10-01T15:17:10Z

@@ -0,0 +1,95 @@
+#!/bin/sh
+# Run the pinned clang-tidy through CMake in the CI container.


nit: I find this comment a bit complicated. I suggest:

# Run clang-tidy through CMake in the CI container. # # This builds dd-trace-cpp-objects with DD_TRACE_ENABLE_CLANG_TIDY=ON. # CMakeLists.txt sets CXX_CLANG_TIDY on that target, so tidy runs with # the real compile line. # # Usage: bin/check-tidy

xlamorlette-datadog · 2026-10-01T15:33:30Z

+REPO_ROOT=$(cd "$SCRIPT_DIR/.." && pwd)
+cd "$REPO_ROOT"
+
+# Keep this in sync with .github/workflows/dev.yml and cmake/compiler/clang.cmake.


nit:

I think that stating that the clang version must be the same everywhere is obvious for C++ developers, and so could be dropped.

It seems to me that the cmake/compiler/clang.cmake reference is a mistake.

I would rather state that the CI image digest must be in sync with .github/workflows/*.yml (this digest is duplicated many times, this is bad, but this was made long ago and is out of scope ;-) ).

xlamorlette-datadog · 2026-10-01T15:36:47Z

+fi
+
+build_dir=${BUILD_DIR:-.clang-tidy-build}
+tidy=clang-tidy-$CLANG_TIDY_VERSION


nit, opt, personal preference: I would prefer to name this variable clang_tidy, I think it would be clearer for C++ developers

xlamorlette-datadog · 2026-10-01T15:57:24Z

+    exit 1
+fi
+
+# Reconfigure so CXX_CLANG_TIDY is attached to dd-trace-cpp-objects, then


Instead of compiling with clang-tidy step, I think we could rather only run clang-tidy directly.
This would remove the added lines in CMakeLists.txt.
Here also, the call would be simpler:

bin/with-toolchain llvm cmake . -B "$build_dir" --preset ci-clang run-clang-tidy-$CLANG_TIDY_VERSION -p "$build_dir" -clang-tidy-binary "$tidy" \ -quiet "^$REPO_ROOT/src/"

By the way, it seems that files in include/ are currently not checked.

Moreover, I think this would ease adding, possibly incrementally, the test/ folder. We need to add this, probably in a follow-up PR.

What do you think?

xlamorlette-datadog · 2026-10-01T16:05:19Z

    apt-get update && apt-get upgrade --yes && \
    apt-get install --yes \
-        wget build-essential clang sed gdb clang-format git ssh shellcheck \
+        wget build-essential clang sed gdb clang-format clang-tidy git ssh shellcheck \


nit, opt, pedantic: By the way, please sort packages alphabetically.

xlamorlette-datadog · 2026-10-01T16:12:05Z

+# compile that target. CMake passes the exact ci-clang compile line
+# (including -stdlib=libc++) to tidy. First-party src/ only; no extra-args.
+bin/with-toolchain llvm cmake . -B "$build_dir" --preset ci-clang \
+    -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \


It seems this is redundant with the added line (line 42) in CMakePresets.json.

xlamorlette-datadog · 2026-10-01T16:16:29Z

  before pushing changes.
 - [check-format](check-format) verifies that the source code is formatted as
  [format](format) prefers.
+- [check-tidy](check-tidy) runs **clang-tidy-14** (pinned, same major as the


nit: I would prefer a much simpler and terser comment, without bold, without version, without internal technical details: runs Clang Tidy.

xlamorlette-datadog · 2026-10-01T16:22:04Z

+
+## Static Analysis
+
+C++ is analyzed with **clang-tidy-14** (pinned; same major as `clang-format-14`)


We need a much simpler and direct documentation, without internal technical details. I suggest:

C++ code is analyzed with Clang Tidy. Run this with the following command: \```shell bin/check-tidy \```

xlamorlette-datadog · 2026-10-01T16:37:04Z

+# readability-redundant-member-init
+# readability-simplify-boolean-expr
+WarningsAsErrors: '*'
+HeaderFilterRegex: '^src/'


It seems this never matches because CMake passes absolute paths, so Clang Tidy sees /…/src/datadog/[xxx].h.
I think we should rather remove this HeaderFilterRegex option and add the header option to run-clang-tidy:

run-clang-tidy-$CLANG_TIDY_VERSION -p "$build_dir" -clang-tidy-binary "$tidy" \ -header-filter "^$REPO_ROOT/(src|include)/" -quiet "^$REPO_ROOT/src/"

Enable clang-tidy tooling

2cfeebf

Co-authored-by: Cursor <cursoragent@cursor.com>

colin-higgins requested review from a team as code owners September 24, 2026 14:54

colin-higgins requested review from cataphract and removed request for a team September 24, 2026 14:54

Install clang-tidy-14 in the CI verify job

4b02832

Co-authored-by: Cursor <cursoragent@cursor.com>

chatgpt-codex-connector Bot reviewed Sep 24, 2026

View reviewed changes

colin-higgins and others added 4 commits September 24, 2026 11:46

Pin clang-tidy-14 and run it in the CI container

17cb3f5

Co-authored-by: Cursor <cursoragent@cursor.com>

Limit clang-tidy to configured src/ sources

bb72b52

Co-authored-by: Cursor <cursoragent@cursor.com>

Run clang-tidy via CMake CXX_CLANG_TIDY

7f2eedd

Co-authored-by: Cursor <cursoragent@cursor.com>

Defer clang-tidy checks with existing findings

70b56c4

Co-authored-by: Cursor <cursoragent@cursor.com>

xlamorlette-datadog requested changes Oct 1, 2026

View reviewed changes

This was referenced Oct 1, 2026

Enable clang-tidy tooling DataDog/nginx-datadog#443

Open

Enable clang-tidy tooling DataDog/httpd-datadog#91

Open

		@@ -0,0 +1,95 @@
		#!/bin/sh
		# Run the pinned clang-tidy through CMake in the CI container.


		## Static Analysis

		C++ is analyzed with clang-tidy-14 (pinned; same major as `clang-format-14`)

Conversation

colin-higgins commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pr-commenter Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Explanation

More details about the CI and significant changes

Uh oh!

cataphract commented Sep 24, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

xlamorlette-datadog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

colin-higgins commented Sep 24, 2026 •

edited

Loading

pr-commenter Bot commented Sep 24, 2026 •

edited

Loading