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

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