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 <[email protected]>
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
3a96cdc 
/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.

This commit:

- Adds std::forward and std::move several places to preserve lvalue/rvalue
  status.

- Defines 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.

- Changes C++ code generator to pass arguments as `M0::Fwd<1>(arg1)` instead of
  `arg1` in generated code. This change can be verified by diffing a generated
  file like build/src/ipc/capnp/mining.capnp.proxy-client.c++ in the bitcoin
  build before and after this change.

Unlike the previous commit which resolved bugprone-move-forwarding-reference
errors by just switching std::move to std::forward, this commit required more
changes because just switching from std::move to std::forward in the
type-interface.h CustomBuildField function would lead to another error in the
CustomMakeProxyServer function there which is expecting an rvalue, not an
lvalue.

That error could have alternately been fixed by changing CustomMakeProxyServer
to accept lvalues and copy the shared_ptr. But this would be slightly less
efficient and it is better to resolve the problem at the root and just use
perfect forwarding everywhere, all the way up the call stack. This required the
changes to the code generator and clientInvoke described above.
@ryanofsky
clang-tidy: Suppress bitcoin-nontrivial-threadlocal error
57a65b8 
/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.

@ryanofsky
Copy link
Collaborator Author

re: #172 (comment)

This might help more people understand the code.

Thanks, added new information above to code comments and commit messages.

Updated a5bff37 -> 57a65b8 (pr/tidy.3 -> pr/tidy.4, compare) updating these comments

@Sjors
Copy link
Member

Sjors commented May 23, 2025 •
edited
Loading

ACK 57a65b8

I studied the second commit more, along with some of mp/gen.cpp, thanks for the extra comments.

I wrote:

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

The original nontrivial-threadlocal check was added in bitcoin/bitcoin#30146 and reviewed by @maflcko and @hebasto, both already on this thread.

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

@Bambibrown Bambibrown Bambibrown approved these changes

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

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