Skip to content

refactor: fix warnings from clang-tidy-20 and bitcoin-tidy #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

refactor: fix warnings from clang-tidy-20 and bitcoin-tidy #172

wants to merge 3 commits into from

Conversation

ryanofsky
Copy link
Collaborator

@ryanofsky ryanofsky commented May 12, 2025 •
edited
Loading

Warnings are described in commit messages and were reported by Sjors in bitcoin/bitcoin#31802 (comment)

@ryanofsky ryanofsky mentioned this pull request May 12, 2025
Draft
@hebasto
Copy link
Member

hebasto commented May 12, 2025

Concept ACK.

maflcko
maflcko reviewed May 13, 2025
View reviewed changes
ryanofsky added 3 commits May 15, 2025 11:24
@ryanofsky
clang-tidy: Fix bugprone-move-forwarding-reference errors
c1e8c1a 
Reported by Sjors Provoost <sjors@sprovoost.nl>
bitcoin/bitcoin#31802 (comment)

/ci_container_base/include/mp/proxy-io.h:252:16: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
/ci_container_base/include/mp/proxy-io.h:336:18: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
/ci_container_base/include/mp/util.h:186:12: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
https://cirrus-ci.com/task/6187773452877824?logs=ci#L4712
@ryanofsky
clang-tidy: Fix bugprone-move-forwarding-reference error
e922bb8 
/ci_container_base/include/mp/type-interface.h:47:75: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
https://cirrus-ci.com/task/6187773452877824?logs=ci#L4712

Error was caused by mp/type-interface.h CustomBuildField field using std::move
instead of std::forward.

Just switching from std::move to std::forward, however, would lead to another
error in the CustomMakeProxyServer function which is expecting an rvalue, not
an lvalue.

That error could be fixed by changing CustomMakeProxyServer to accept lvalues
and copy the shared_ptr, which would have a slight performance cost. But it is
better to resolve the problem at the root and switch to perfect forwarding
everywhere, all the way up the call stack. This required some small changes to
the code generator and clientInvoke.
@ryanofsky
clang-tidy: Suppress bitcoin-nontrivial-threadlocal error
a5bff37 
/ci_container_base/include/mp/proxy-io.h:637:1: error: Variable with non-trivial destructor cannot be thread_local. [bitcoin-nontrivial-threadlocal,-warnings-as-errors]
https://cirrus-ci.com/task/6187773452877824?logs=ci#L4720
@ryanofsky
Copy link
Collaborator Author

ryanofsky commented May 15, 2025 •
edited
Loading

Rebased 8d8e9f6 -> e6a71ae (pr/tidy.1 -> pr/tidy.2, compare) fixing typos and conflict with #165
Updated e6a71ae -> a5bff37 (pr/tidy.2 -> pr/tidy.3, compare) expanding a comment

@Sjors
Copy link
Member

Sjors commented May 16, 2025 •
edited
Loading

I added the latest version of this to bitcoin/bitcoin#31802, the tidy job is still happy.

Some background for the first two commits: https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html

With that in mind the first commit seems fine, the second one e922bb8 is a bit over my head due to the changes in src/mpgen.cpp which I'm not familiar with.

I don't have an informed opinion on the last commit a5bff37 that suppresses bitcoin-nontrivial-threadlocal, but it has a comment with rationale.

@ryanofsky
Copy link
Collaborator Author

Thanks @Sjors. Now that this git repo is a subtree and a bitcoin-core project, I would like to (1) stop my previous practice of merging PRs like this that do not have any ACKs and (2) try to encourage more review to happen earlier in the upstream PR's in this repository rather than later in the downstream PRs in the bitcoin repository.

So would you feel comfortable reviewing more and maybe giving an ACK along the lines of...

  • Testing this PR and confirming it fixes the warnings
  • Looking at the code and confirming it only disables one bitcoin-tidy warning and resolves several bugprone-move-forwarding-reference warnings by changing std::move to std::forward in some places and changing lvalue references to rvalue references other places

... or however you might actually review it? I am happy to answer any questions that could help clarify things. The second commit should be the only complicated one here, and I can break it down. It is:

  • Adding std::forward and std::move several places to preserve lvalue/rvalue status.
  • Defining a FunctionTraits::Fwd<...>(...) helper which does same thing as std::forward except it takes parameter number instead of a parameter type so generated code doesn't need as verbose as std::forward calls would be.
  • Changing C++ code generator to pass arguments as M0::Fwd<1>(arg1) instead of arg1 in generated code. You could verify this by diffing a generated file like build/src/ipc/capnp/mining.capnp.proxy-client.c++ in the bitcoin build before and after this change.

Maybe I should add some of these explanations in code comments or commit messages too. Would welcome any feedback.

@Sjors
Copy link
Member

Sjors commented May 16, 2025

Maybe I should add some of these explanations in code comments

This might help more people understand the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maflcko maflcko maflcko left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ryanofsky @hebasto @Sjors @maflcko