Sponsored Content
Skip to content

chore: Enable clang-tidy misc checks - #6655

Merged
bthomee merged 20 commits into
XRPLF:developfrom
godexsoft:chore/clang-tidy-checks-misc
Mar 31, 2026
Merged

chore: Enable clang-tidy misc checks#6655
bthomee merged 20 commits into
XRPLF:developfrom
godexsoft:chore/clang-tidy-checks-misc

Conversation

@godexsoft

@godexsoft godexsoft commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

This PR enables various checks:

  • llvm-namespace-comment
  • misc-const-correctness
  • misc-definitions-in-headers
  • misc-throw-by-value-catch-by-reference
  • misc-redundant-expression

API Impact

No impact.

@godexsoft godexsoft added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Mar 25, 2026
@github-actions

Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@godexsoft
godexsoft requested review from kuznetsss and removed request for kuznetsss March 27, 2026 04:34
@github-actions

Copy link
Copy Markdown

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

continue-on-error: true
env:
TARGETS: ${{ inputs.files != '' && inputs.files || 'src tests' }}
TARGETS: ${{ inputs.files != '' && inputs.files || '^(?!.*src/tests/libxrpl/protocol_autogen/).*(/src/|/tests/).*\.cpp$' }}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the easiest way for now, we don't want to run tidy on generated code, eventually we may want to enable it but i think it's best left for last moment when all other code is fully compliant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we exclude generated code paths in .clang-tidy file instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of such option in clang-tidy. The closest thing i know of is header filter and exclude filter but that only affects #included code, not inputs directly consumed by clang-tidy

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mathbunnyru to the rescue here.

@mathbunnyru mathbunnyru Mar 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found a way - put a second .clang-tidy file in the directory you want to exclude with

# This disables all checks for this directory and its subdirectories
Checks: '-*'
InheritParentConfig: false

Source: https://stackoverflow.com/a/75586089

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! this is actually a great idea, i will try, thanks for the suggestion

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bthomee I rescued 😂

@codecov

codecov Bot commented Mar 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.78501% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (7f53351) to head (459be62).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
include/xrpl/resource/detail/Logic.h 55.6% 8 Missing ⚠️
src/libxrpl/nodestore/backend/RocksDBFactory.cpp 41.7% 7 Missing ⚠️
src/libxrpl/json/json_reader.cpp 68.4% 6 Missing ⚠️
src/libxrpl/nodestore/DatabaseRotatingImp.cpp 55.6% 4 Missing ⚠️
src/libxrpl/rdb/SociDB.cpp 20.0% 4 Missing ⚠️
src/libxrpl/basics/Log.cpp 57.1% 3 Missing ⚠️
src/libxrpl/beast/insight/StatsDCollector.cpp 0.0% 3 Missing ⚠️
src/libxrpl/core/detail/JobQueue.cpp 83.3% 3 Missing ⚠️
src/libxrpl/shamap/SHAMapSync.cpp 66.7% 3 Missing ⚠️
src/libxrpl/basics/Archive.cpp 0.0% 2 Missing ⚠️
... and 20 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6655     +/-   ##
=========================================
- Coverage     81.5%   81.4%   -0.1%     
=========================================
  Files          999     999             
  Lines        74459   74458      -1     
  Branches      7563    7577     +14     
=========================================
- Hits         60652   60613     -39     
- Misses       13807   13845     +38     
Files with missing lines Coverage Δ
include/xrpl/basics/BasicConfig.h 89.7% <100.0%> (ø)
include/xrpl/basics/IntrusiveRefCounts.h 92.7% <100.0%> (ø)
include/xrpl/basics/SlabAllocator.h 93.3% <100.0%> (ø)
include/xrpl/basics/StringUtilities.h 100.0% <100.0%> (ø)
include/xrpl/basics/base_uint.h 94.3% <100.0%> (ø)
include/xrpl/basics/hardened_hash.h 100.0% <100.0%> (ø)
include/xrpl/basics/random.h 100.0% <100.0%> (ø)
include/xrpl/beast/asio/io_latency_probe.h 96.5% <100.0%> (ø)
include/xrpl/beast/utility/Zero.h 100.0% <ø> (ø)
include/xrpl/core/ClosureCounter.h 100.0% <100.0%> (ø)
... and 146 more

... and 16 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -23,7 +23,7 @@ operator==(Config const& lhs, Config const& rhs)
return lhs.autoConnect == rhs.autoConnect && lhs.peerPrivate == rhs.peerPrivate &&
lhs.wantIncoming == rhs.wantIncoming && lhs.inPeers == rhs.inPeers &&
lhs.maxPeers == rhs.maxPeers && lhs.outPeers == rhs.outPeers &&
lhs.features == lhs.features && lhs.ipLimit == rhs.ipLimit &&

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a copypasta bug caught 👍

@godexsoft
godexsoft marked this pull request as ready for review March 31, 2026 01:27
@godexsoft godexsoft removed the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Mar 31, 2026
@bthomee
bthomee self-requested a review March 31, 2026 09:58
@kuznetsss
kuznetsss requested a review from Copilot March 31, 2026 12:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@godexsoft godexsoft added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 31, 2026
@bthomee
bthomee added this pull request to the merge queue Mar 31, 2026
@bthomee
bthomee removed this pull request from the merge queue due to a manual request Mar 31, 2026
@godexsoft
godexsoft requested a review from mathbunnyru March 31, 2026 15:02

@mathbunnyru mathbunnyru left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked random places, overall LGTM

@bthomee
bthomee added this pull request to the merge queue Mar 31, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 31, 2026
@bthomee

bthomee commented Mar 31, 2026

Copy link
Copy Markdown
Collaborator

@godexsoft looks like more clang-tidy fixes are needed.

@bthomee
bthomee enabled auto-merge March 31, 2026 17:14
@bthomee
bthomee added this pull request to the merge queue Mar 31, 2026
Merged via the queue into XRPLF:develop with commit 2502bef Mar 31, 2026
3 checks passed
bthomee pushed a commit that referenced this pull request Apr 1, 2026
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants