backport: bitcoin/bitcoin#26705, #26706 - clang-tidy: for all headers and related fixes #7103
Conversation
|
WalkthroughThis pull request refactors member initialization across multiple files, moving from constructor initializer lists to in-class member initializers (C++11 feature). The changes affect cryptographic classes (RIPEMD160, SHA1, SHA256, SHA512), database wrappers, cache implementations, and wallet-related structures. Additionally, the RPC utilities are updated to support named-argument awareness in help text generation by introducing an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (31)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (18)
🧰 Additional context used📓 Path-based instructions (3)src/**/*.{cpp,h,hpp,cc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/wallet/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{test,wallet/test}/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-12-22T15:42:48.595ZApplied to files:
📚 Learning: 2025-01-14T09:06:19.717ZApplied to files:
📚 Learning: 2025-09-09T21:36:31.504ZApplied to files:
📚 Learning: 2025-09-09T21:36:58.969ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-01-14T09:05:12.032ZApplied to files:
🧬 Code graph analysis (6)src/crypto/sha1.h (3)
src/crypto/sha256.cpp (1)
src/zmq/zmqabstractnotifier.h (1)
src/crypto/sha1.cpp (1)
src/crypto/ripemd160.h (3)
src/crypto/sha512.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (11)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
knst
changed the title
modernize-use-default-member-init in headers and force to check all headersmodernize-use-default-member-init in headers and force to check all headers
fad56f7 doc: Properly report optional RPC args (MarcoFalke) fa09cb6 refactor: Introduce is_top_level_arg (MarcoFalke) fab92a5 refactor: Remove const to fix performance-move-const-arg clang-tidy errors (MarcoFalke) Pull request description: `OMITTED_NAMED_ARG` and `OMITTED` are a confusing burden: * It puts the burden on developers to pick the right one of the two * They can be interchanged without introducing a compile failure or other error * Picking the wrong one is leading to incorrect docs * They are redundant, because the correct one can already be determined by the surrounding type Fix all issues by making them an alias of each other; Pick the right one based on the outer type. ACKs for top commit: fanquake: ACK fad56f7 Tree-SHA512: 6e7193a05a852ba1618a9cb3261220c7ad3282bc5595325c04437aa811f529a88e2851e9c7dbf9878389b8aa42e98f8817b7eb0260fbb9e123da0893cbae6ca2
knst
changed the title
modernize-use-default-member-init in headers and force to check all headers![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/hdchain.h (1)
6-10: Fix clang-format failure in this header (CI is currently blocked).
CI reports a clang-format diff atsrc/wallet/hdchain.hLine 9; please run the repo’s clang-format (or the same “diff format check” target) and update the file accordingly.
🤖 Fix all issues with AI agents
In `@src/test/blockencodings_tests.cpp`:
- Around line 16-17: Replace the local extern declaration "extern RecursiveMutex
cs_main;" in blockencodings_tests.cpp with the canonical declaration by adding
an `#include` for validation.h and removing the extern; specifically, remove the
local extern RecursiveMutex cs_main; and ensure the file includes validation.h
so cs_main (and its RecursiveMutex type) is referenced from the authoritative
declaration rather than a local extern.
🧹 Nitpick comments (1)
src/wallet/bdb.cpp (1)
321-330: Watch for redundant init:pdb(nullptr)may be unnecessary now thatpdb{nullptr}exists in-class.
If clang-tidy is configured to warn on redundant member initializers, consider removingpdb(nullptr)from the ctor init-list and rely on the in-class default (keepingm_cursor(nullptr)andm_database(database)as-is).Proposed tweak (only if clang-tidy complains)
-BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : pdb(nullptr), m_cursor(nullptr), m_database(database) +BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : m_cursor(nullptr), m_database(database) { database.AddRef(); database.Open(); fReadOnly = read_only; fFlushOnClose = fFlushOnCloseIn; env = database.env.get(); pdb = database.m_db.get(); strFile = fs::PathToString(database.m_filename); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
src/.clang-tidysrc/bls/bls.hsrc/coinjoin/coinjoin.hsrc/crypto/ripemd160.cppsrc/crypto/ripemd160.hsrc/crypto/sha1.cppsrc/crypto/sha1.hsrc/crypto/sha256.cppsrc/crypto/sha256.hsrc/crypto/sha512.cppsrc/crypto/sha512.hsrc/cuckoocache.hsrc/dbwrapper.hsrc/evo/mnhftx.hsrc/key.hsrc/node/blockstorage.hsrc/policy/fees.cppsrc/policy/fees.hsrc/rpc/util.cppsrc/rpc/util.hsrc/serialize.hsrc/span.hsrc/streams.hsrc/support/lockedpool.cppsrc/support/lockedpool.hsrc/test/blockencodings_tests.cppsrc/test/util/net.hsrc/tinyformat.hsrc/torcontrol.cppsrc/torcontrol.hsrc/txmempool.hsrc/univalue/include/univalue_utffilter.hsrc/wallet/bdb.cppsrc/wallet/bdb.hsrc/wallet/db.hsrc/wallet/hdchain.hsrc/wallet/wallet.hsrc/zmq/zmqabstractnotifier.hsrc/zmq/zmqnotificationinterface.cppsrc/zmq/zmqnotificationinterface.h
💤 Files with no reviewable changes (4)
- src/coinjoin/coinjoin.h
- src/node/blockstorage.h
- src/txmempool.h
- src/policy/fees.cpp
✅ Files skipped from review due to trivial changes (2)
- src/evo/mnhftx.h
- src/bls/bls.h
🚧 Files skipped from review as they are similar to previous changes (18)
- src/span.h
- src/support/lockedpool.h
- src/wallet/wallet.h
- src/univalue/include/univalue_utffilter.h
- src/crypto/sha1.h
- src/support/lockedpool.cpp
- src/cuckoocache.h
- src/zmq/zmqnotificationinterface.h
- src/torcontrol.cpp
- src/crypto/sha512.cpp
- src/tinyformat.h
- src/crypto/ripemd160.h
- src/wallet/db.h
- src/serialize.h
- src/key.h
- src/crypto/sha256.h
- src/crypto/sha1.cpp
- src/test/util/net.h
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/policy/fees.hsrc/wallet/hdchain.hsrc/wallet/bdb.hsrc/test/blockencodings_tests.cppsrc/zmq/zmqabstractnotifier.hsrc/crypto/ripemd160.cppsrc/torcontrol.hsrc/streams.hsrc/wallet/bdb.cppsrc/rpc/util.cppsrc/crypto/sha512.hsrc/crypto/sha256.cppsrc/dbwrapper.hsrc/rpc/util.hsrc/zmq/zmqnotificationinterface.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/hdchain.hsrc/wallet/bdb.hsrc/wallet/bdb.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/blockencodings_tests.cpp
🧠 Learnings (12)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request `#6543` is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (`#6718`), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/policy/fees.hsrc/test/blockencodings_tests.cpp
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/policy/fees.hsrc/wallet/hdchain.hsrc/wallet/bdb.hsrc/zmq/zmqabstractnotifier.hsrc/torcontrol.hsrc/streams.hsrc/crypto/sha512.hsrc/dbwrapper.hsrc/rpc/util.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/wallet/hdchain.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/wallet/hdchain.hsrc/wallet/bdb.hsrc/wallet/bdb.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/wallet/bdb.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/test/blockencodings_tests.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/test/blockencodings_tests.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/zmq/zmqabstractnotifier.h
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/zmq/zmqabstractnotifier.h
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/util.cppsrc/rpc/util.h
📚 Learning: 2025-01-14T09:06:19.717Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
Applied to files:
src/crypto/sha256.cpp
🧬 Code graph analysis (5)
src/zmq/zmqabstractnotifier.h (1)
src/zmq/zmqabstractnotifier.cpp (1)
CZMQAbstractNotifier(11-14)
src/crypto/ripemd160.cpp (1)
src/crypto/ripemd160.h (1)
CRIPEMD160(12-26)
src/crypto/sha512.h (3)
src/crypto/ripemd160.h (1)
bytes(17-17)src/crypto/sha1.h (1)
bytes(17-17)src/crypto/sha256.h (1)
bytes(18-18)
src/dbwrapper.h (1)
src/wallet/bdb.cpp (2)
ssKey(492-492)ssValue(493-493)
src/rpc/util.h (1)
src/rpc/util.cpp (6)
ToDescriptionString(503-518)ToDescriptionString(503-503)ToDescriptionString(520-523)ToDescriptionString(520-520)ToDescriptionString(655-716)ToDescriptionString(655-655)
🪛 GitHub Actions: Clang Diff Format Check
src/wallet/hdchain.h
[error] 9-9: Clang-format differences detected. Update formatting (diff_output.txt) to align with clang-format rules.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (21)
src/crypto/sha512.h (1)
15-27: Good: default-initializebytesto avoid indeterminate state and align with other hashers.
This matches thebytes{0}pattern already used insrc/crypto/sha1.h,src/crypto/sha256.h, andsrc/crypto/ripemd160.h, and should satisfymodernize-use-default-member-initwhile keepingSize()behavior unchanged.src/torcontrol.h (1)
88-92: Good: default-initializeb_connin-class to satisfy clang-tidy and avoid redundant ctor init.
This aligns with the backport/clang-tidy objective and keeps behavior unchanged (still starts asnullptr).src/zmq/zmqabstractnotifier.h (2)
38-38: Constructor init list simplification is behavior-preserving here.Keeping
outbound_message_high_water_mark(DEFAULT_ZMQ_SNDHWM)while movingpsocketinit in-class matches the clang-tidy goal and should remain equivalent for the default ctor.
81-81: Good:psocketnow defaults to nullptr for all constructors.This makes the destructor invariant (
assert(!psocket)insrc/zmq/zmqabstractnotifier.cpp) safer across future ctor additions and aligns withmodernize-use-default-member-init.src/dbwrapper.h (1)
82-83: Good clang-tidy-driven cleanup (default member init keeps behavior stable).
Movingsize_estimateto an in-class{0}initializer and removing it from the ctor init-list keepsClear()semantics intact and avoids future “forgot to init” footguns.Also applies to: 88-94
src/policy/fees.h (3)
243-246: LGTM!The in-class brace initializers follow the
modernize-use-default-member-initclang-tidy check correctly. The syntax combiningGUARDED_BYannotations with brace initialization is valid C++.
248-253: LGTM!The struct member initializers correctly follow the
modernize-use-default-member-initpattern. The empty constructor body is appropriate for a backport matching upstream Bitcoin Core.
263-264: LGTM!Consistent application of in-class brace initialization for the tracking counters.
src/zmq/zmqnotificationinterface.cpp (1)
25-27: Constructor change is correct and safe.Verification confirms
pcontexthas a default member initializer (void* pcontext{nullptr};in the header, line 48), and there is only one constructor, which is now empty. This ensurespcontextis properly initialized tonullptron construction, satisfying theassert(!pcontext)check inInitialize(). The removal of the explicit initializer list aligns with modern C++20 practices using default member initialization.src/crypto/ripemd160.cpp (1)
242-245: Constructor now relies on in-class init forbytes(OK) andInitialize(s)for state (OK).This matches the modernize-use-default-member-init pattern:
sis fully initialized viaripemd160::Initialize(s)andbytesis already defaulted to0insrc/crypto/ripemd160.h.bufstays uninitialized, but the currentWrite/Finalizeflow only uses it after writing into it, so this should be safe. Based on learnings, keeping this aligned with upstream is preferred.src/crypto/sha256.cpp (1)
693-696: Thebytesmember is properly zero-initialized in the header.The header file (src/crypto/sha256.h, line 18) has
uint64_t bytes{0};, confirming thatbytesis zero-initialized via an in-class initializer. The constructor correctly relies on this for proper initialization.src/streams.h (1)
626-679: LGTM: safe move to in-class default initialization for stream positions.
This aligns withmodernize-use-default-member-initand keeps the constructor initializer list focused on non-trivial members.src/wallet/bdb.h (1)
191-199: LGTM:pdb/activeTxnin-class nullptr initialization.
One follow-up: ensure clang-tidy config won’t flag redundant ctor init for members that now have in-class defaults (e.g.pdb(nullptr)inbdb.cpp).src/wallet/hdchain.h (1)
12-21: No semantic shift risk found in practice.The removal of the explicit constructor does technically make
CHDAccountan aggregate, but a codebase search found no brace-initialization patterns likeCHDAccount{x, y}. All existing call sites use either default initialization (CHDAccount acc;) or explicit default constructor calls (CHDAccount()), both of which remain valid. No behavior change is possible.src/rpc/util.cpp (3)
596-604:ToString()now treats argument descriptions as “named” (Line 599) — confirm this matches expected RPC help text.
arg.ToDescriptionString(/*is_named_arg=*/true)will now append “, optional” for omitted named args; this is a user-visible doc output change.
655-716: Signature + semantics change inRPCArg::ToDescriptionString(bool)is fine; ensureis_named_argis only set where appropriate.In particular,
OMITTEDnow conditionally emits “optional” only whenis_named_argis true (Lines 701-705). That’s subtle and easy to misapply at call sites.
357-409: Formatting logic is correct and properly handles help output for obj/arr vs scalar args.The verification confirms the implementation matches the intended design:
- Zero-arg calls to
ToDescriptionString()found at lines 608 and 611 are onRPCResultsandRPCExamplestypes, notRPCArg, so they are expected and correct.- Boolean-arg calls to
RPCArg::ToDescriptionString(/*is_named_arg=*/...)are properly present at lines 379, 384, 399, and 599.- The
is_top_level_argearly-return for scalar types (line 371) and conditional empty RHS for top-level obj/arr (lines 384, 399) correctly avoid duplicating descriptions already emitted byRPCHelpMan::ToString().The code is ready as-is.
src/rpc/util.h (3)
188-225:RPCArgctor +ToDescriptionString(bool)signature change: verify no stale declarations/usages remain.Given the by-value ctor params and new
ToDescriptionString(bool)API, compilation will catch most issues—but it’s worth confirming there aren’t any ambiguous/accidentally-reordered ctor call sites.Also applies to: 249-250
276-324:RPCResultby-value constructors: OK; confirm call sites didn’t rely on implicit conversions.Switching to by-value/move can change overload resolution (especially with
{}initializer lists). Quick scan of construction sites is recommended.
156-178: Fallback refactor is well-designed for backward compatibility.The
Fallback = std::variant<Optional, DefaultHint, Default>change toRPCArgis sound. Existing call sites already pass all three variant member types (RPCArg::Optional::NO/OMITTED,RPCArg::Default{...},RPCArg::DefaultHint{...}), and implicit variant conversion handles them transparently.RPCResultis unaffected by this change and requires no updates. TheOMITTED_NAMED_ARGdeprecation alias adds no breaking changes.src/.clang-tidy (1)
29-29: The patternHeaderFilterRegex: '.'correctly selects all headers—no change needed.
HeaderFilterRegexusesllvm::Regex, which performs substring matching by default. The pattern.matches any file path containing at least one character, which covers all normal headers. Your concern about full-string matching requiring 4-character patterns is not applicable here.If preferred for clarity,
.*is more idiomatic and equally effective, but the current pattern works as intended.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/test/blockencodings_tests.cpp
Outdated
| extern RecursiveMutex cs_main; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for canonical cs_main declaration (non-extern)
rg -n '^\s*RecursiveMutex\s+cs_main' --type cpp --type hRepository: dashpay/dash
Length of output: 102
🏁 Script executed:
# Search for where cs_main is declared in headers
rg -n 'RecursiveMutex\s+cs_main' --type hRepository: dashpay/dash
Length of output: 773
🏁 Script executed:
# Check current includes in blockencodings_tests.cpp
head -30 src/test/blockencodings_tests.cppRepository: dashpay/dash
Length of output: 855
🏁 Script executed:
# Search for other test files using extern RecursiveMutex cs_main
rg -l 'extern\s+RecursiveMutex\s+cs_main' src/test --type cppRepository: dashpay/dash
Length of output: 89
🏁 Script executed:
# Check if validation.h includes or declares cs_main
rg -n 'cs_main' src/validation.h | head -20Repository: dashpay/dash
Length of output: 1357
🏁 Script executed:
# Check what setup_common.h includes
head -50 src/test/util/setup_common.hRepository: dashpay/dash
Length of output: 1479
🏁 Script executed:
# Check if setup_common.h already includes validation.h (transitively)
rg 'validation\.h' src/test/util/setup_common.hRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Verify no circular dependency would occur by checking validation.h's includes
rg '#include.*test.*setup_common' src/validation.hRepository: dashpay/dash
Length of output: 38
Include validation.h to reference the canonical cs_main declaration instead of using a local extern statement
The file currently relies on a local extern RecursiveMutex cs_main; declaration at lines 16-17, but the canonical declaration is in src/validation.h:115. Following the codebase convention (used in all other headers and implementations), include validation.h directly and remove the local extern. This avoids brittle signature drift if the canonical declaration changes.
Recommended fix
`#include` <blockencodings.h>
`#include` <chainparams.h>
`#include` <consensus/merkle.h>
`#include` <pow.h>
`#include` <streams.h>
`#include` <test/util/txmempool.h>
+#include <validation.h>
`#include` <test/util/setup_common.h>
`#include` <boost/test/unit_test.hpp>
-extern RecursiveMutex cs_main;🤖 Prompt for AI Agents
In `@src/test/blockencodings_tests.cpp` around lines 16 - 17, Replace the local
extern declaration "extern RecursiveMutex cs_main;" in blockencodings_tests.cpp
with the canonical declaration by adding an `#include` for validation.h and
removing the extern; specifically, remove the local extern RecursiveMutex
cs_main; and ensure the file includes validation.h so cs_main (and its
RecursiveMutex type) is referenced from the authoritative declaration rather
than a local extern.
|
build fails |
…it` in headers and force to check all headers b0e9169 clang-tidy: Force to check all headers (Hennadii Stepanov) 96ee992 clang-tidy: Fix `modernize-use-default-member-init` in headers (Hennadii Stepanov) Pull request description: This PR: - fixes the only [remained](bitcoin#26705 (comment)) check in headers, i.e., `modernize-use-default-member-init` - forces `clang-tidy` check all headers Closes bitcoin#26703. ACKs for top commit: MarcoFalke: review ACK b0e9169 🍹 Tree-SHA512: 4d33fe873094914541ae81968cdb4e7a7a01b3fdd4f25bc6daa8a53f45dab80565a5b3607ddc338f122369ca5a0a2d0d09c8e78cabe1beb6bd50c115bc5c5210
…n for cs_main forward declarations
3 participants
Issue being fixed or feature implemented
This PR:
modernize-use-default-member-initclang-tidycheck all headersWhat was done?
It's more than 100 failures for dash specific code; it is extracted from #7100 to its own PR to validate clang-tidy failures and address them one-by-one.
How Has This Been Tested?
Run CI & fix failures
Breaking Changes
N/A
Checklist: