Skip to content

type-number: fix clang-tidy modernize-use-nullptr#292

Merged
ryanofsky merged 1 commit into
bitcoin-core:masterfrom
ViniciusCestarii:type-number-use-nullptr
Jun 9, 2026
Merged

type-number: fix clang-tidy modernize-use-nullptr#292
ryanofsky merged 1 commit into
bitcoin-core:masterfrom
ViniciusCestarii:type-number-use-nullptr

Conversation

@ViniciusCestarii

Copy link
Copy Markdown
Contributor

Prevent warning --modernize-use-nullptr when using clang-tidy on downstream projects (Noticed when pushing 2140-dev/bitcoin#42, this change was also necessary for bitcoin/bitcoin#29409 so CI doens't error)

@DrahtBot

DrahtBot commented Jun 9, 2026

Copy link
Copy Markdown

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
Stale ACK ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

@maflcko

maflcko commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Looks like CI fails in mptest, which may be unrelated.

Also, I wonder why the CI passes fine without this change, when it does claim to run clang-tidy:

-DMP_ENABLE_CLANG_TIDY=ON

@ViniciusCestarii

Copy link
Copy Markdown
Contributor Author

Also, I wonder why the CI passes fine without this change, when it does claim to run clang-tidy:

The CI does run clang-tidy, but it only warns about code that actually gets compiled. The float overload of CustomReadField (the one with = 0) is a template, so it only turns into real code when something uses a float type. libmultiprocess's own foo.capnp has no Float64 field, so that code is never generated and clang-tidy never looks at it. Bitcoin Core's .capnp does use Float64, so mpgen generates the code, and then clang-tidy can see the type and warn with modernize-use-nullptr.

Adding a Float64 field on foo.capnp and foo.h would have triggered this error here. I believe there other types that are not being tested too.

@ViniciusCestarii

Copy link
Copy Markdown
Contributor Author

Looks like CI fails in mptest, which may be unrelated.

The same thing happened on #278 which only changes a markdown file. I will give a look on what's happening

@ryanofsky ryanofsky left a comment

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.

Code review ACK 91a951f. Thanks for the PR. It could also be a good to add a test that would trigger this error. I guess would just need to add a test passing a floating point value.

@ryanofsky ryanofsky merged commit 733c643 into bitcoin-core:master Jun 9, 2026
12 of 13 checks passed
@maflcko

maflcko commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Added test in #294

Sjors added a commit to Sjors/sv2-tp that referenced this pull request Jun 17, 2026
8412fcdc65 Merge bitcoin-core/libmultiprocess#295: Mark Waiter m_cv as guarded by m_mutex
1593ee2d18 Merge bitcoin-core/libmultiprocess#294: test: Add passDouble smoke test
9885d7dd33 Merge bitcoin-core/libmultiprocess#286: proxy-client: fix TSan data race in clientDestroy
fa35501c4f Mark Waiter m_cv as guarded by m_mutex
faaedb11f8 test: Add passDouble smoke test
733c64318d Merge bitcoin-core/libmultiprocess#292: type-number: fix clang-tidy modernize-use-nullptr
9cc3479ab3 Merge bitcoin-core/libmultiprocess#291: cmake: Add `mp_headers` custom target
201abd9e3a Merge bitcoin-core/libmultiprocess#289: cmake: make target_capnp_sources use CURRENT dirs
99820c8aec Merge bitcoin-core/libmultiprocess#279: doc: Add comments to FIELD_* constants in proxy.h
73b985540c Merge bitcoin-core/libmultiprocess#278: doc: Fix and expand design.md
e7e91b2e23 Merge bitcoin-core/libmultiprocess#277: Add std::unordered_set support and a helper BuildList to dedup list build handlers
91a951f59a tidy fix: modernize-use-nullptr
16362f42d0 cmake: Add `mp_headers` custom target
615a94fe3a cmake: document ONLY_CAPNP option in target_capnp_sources
90982f75c6 mpgen: iwyu changes required by previous commit
25bb3e67f3 proxy-client: fix TSan data race in clientDestroy
620f297f31 cmake: make target_capnp_sources use CURRENT dirs
9de4b885aa test: use camelCase + $Proxy.name for FooStruct fields
011b91793d type: add std::unordered_set support
20d19b9644 proxy: add BuildList helper and dedup map/set/vector build handlers
e863c6cdf6 doc: Add comments to FIELD_* constants in proxy.h
18db0ab957 doc: Fix and expand design.md
61de697536 Merge bitcoin-core/libmultiprocess#273: proxy-client: tolerate exceptions from remote destroy during cleanup
9cec9d6ca5 Merge bitcoin-core/libmultiprocess#243: mpgen: support primitive std::optional struct fields
4aaff11374 Merge bitcoin-core/libmultiprocess#238: cmake, ci: updates for recent nixpkgs
2ac55a56b5 Merge bitcoin-core/libmultiprocess#218: Better error and log messages
6de92e1c73 proxy-client: tolerate exceptions from remote destroy during cleanup
90be8354d4 test: regression for ~ProxyClient destroy after peer disconnect
3c69d125a1 Merge bitcoin-core/libmultiprocess#260: event loop: tolerate unexpected exceptions in `post()` callbacks
b8a48c65e6 event loop: tolerate unexpected exceptions in `post()` callbacks
f787863d2c Merge bitcoin-core/libmultiprocess#270: doc: Bump version 10 > 11
a22f602910 doc: Bump version 10 > 11
4eae445d6d debug: Add TypeName() function and log statements for Proxy objects being created and destroyed
f326c5b1b7 logging: Add better logging on IPC server-side failures
6dbfa56a04 mpgen: support primitive std::optional struct fields
8d1277deb5 mpgen refactor: add AccessorType function
db716bbcba mpgen refactor: Move field handling code to FieldList class
db7acb3ce2 ci: Fix shell.nix compatibility with CMake 4.0
91a7759a9a cmake: Fix IWYU in nix by adding CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 8412fcdc659e1379f9b4dea896c26bc1c5f3afa8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants