Repository navigation
Enable readability-braces-around-statements in clang-tidy - #390
Conversation
Recurring review theme: reviewers across dd-trace-cpp and httpd-datadog have repeatedly asked for braces around single-line if/return bodies (dd-trace-cpp#286, dd-trace-cpp#295, dd-trace-cpp#313, httpd-datadog#62). This repo's src/ tree already has no brace-less control statements, so enabling the check needs no source changes here.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Enabling the new brace-enforcement clang-tidy rule without first fixing existing violations in src/datadog/w3c_propagation.cpp (line 151) and src/datadog/platform_util_unix.cpp (lines 183–186) will break CI, because WarningsAsErrors: '*' turns these diagnostics into hard failures for bin/check-tidy.
🤖 Bits Code Review · Commit f7ccec3
|
To allow one-line condition block, such as: It seems we can try: |
BenchmarksBenchmark execution time: 2026-10-23 15:24:40 Comparing candidate commit 6d6d377 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 8 metrics, 0 unstable metrics.
|
Overview
Enable
readability-braces-around-statementsin the shared clang-tidy baseline (.clang-tidy), wired into the existingclang-tidyCI step (bin/check-tidy, run from.github/workflows/dev.yml).Motivation
Reviewers have repeatedly asked contributors to add braces around single-line
if/returnbodies in C++ changes. This is a recurring, mechanical review comment that a clang-tidy check can enforce automatically instead of a human re-raising it on every PR.Additional Notes
This repo's
src/tree (the tree scanned bybin/check-tidy) already has no brace-less control statements, so enabling the check requires no source changes here — it is a config-only change. The companion check is also being proposed inhttpd-datadogandnginx-datadog, which share this.clang-tidybaseline and do have a small number of existing brace-less statements to fix alongside enabling the check there.References
Jira ticket:
Contributor Checklist
Additional context (automated analysis)
This PR was opened by an automated clang-tidy-review workflow that scans PR activity across
DataDog/dd-trace-cpp,DataDog/httpd-datadog, andDataDog/nginx-datadogfor recurring, automatable C++ review themes.Theme: Missing braces around single-line
if/returncontrol-flow bodies (readability-braces-around-statements).Observed in: 4 occurrences across 2 repos / 4 distinct PRs (recurrence bar requires ≥2 distinct PRs):
Verification:
src/andinclude/in this repo were checked (via GitHub content reads ofspan.cpp,trace_segment.cpp,tracer.cpp,trace_sampler.cpp,w3c_propagation.cpp, and targetedsearch_codequeries for brace-lessif (...) return ...;patterns) and found clean of this pattern already, so this change is config-only and should not fail the existingclang-tidyCI job.Not actioned this run (reported for completeness):
readability-magic-numbers— recurring (dd-trace-cpp#286, dd-trace-cpp#330, nginx-datadog#304) but has a high false-positive rate against existing code (e.g. HTTP/protocol version constants, array sizes) and would need case-by-case suppression; skipped per the "require low false-positive rate" guardrail.cppcoreguidelines-pro-type-cstyle-cast) — recurring (dd-trace-cpp#286, nginx-datadog#304) butsrc/datadog/trace_sampler.cpphas several pre-existing functional-style casts (int(rule.mechanism), etc.) and clang-tidy here runs over the wholesrc/tree (no per-diff scoping mechanism exists), so enabling it now would break CI on pre-existing code.new/deleteinstead of RAII — recurring (httpd-datadog#88, nginx-datadog#304) but the observed instances are deliberate C-API interop with pool-scoped lifetimes (APR/ngx pools), not accidental ownership bugs;bugprone-unused-raii(already enabled) covers the genuine bug case.