clang-tidy: fix warnings introduced in version 19 #165

purpleKarrot · 2025-02-28T09:08:38Z

Should fix #153.

include/mp/proxy-types.h

purpleKarrot · 2025-02-28T11:25:05Z

I think the remaining uses of SFINAE should be refactored rather than migrated to requires.

ryanofsky · 2025-02-28T14:25:27Z

Thanks for the changes! Didn't review in detail yet but these all look good.

I think the remaining uses of SFINAE should be refactored rather than migrated to requires.

This probably makes sense but sounds vague. Do you have an example in mind which would make it clear?

purpleKarrot · 2025-02-28T16:14:37Z

@ryanofsky, I am referring to those changes: 0dd3a4e

Above the changed code, there is this comment, which probably should be addressed:

// TODO Possible optimization to speed up compile time:
// https://stackoverflow.com/a/7858971 Using enable_if below to check
// position when unpacking tuple might be slower than pattern matching
// approach in the stack overflow solution

The problem is that I cannot really make sense of the code. It recursively calls callBuild, forwarding all arguments plus one additional one. The first call to callBuild is from handleField, which forwards all its arguments. The last call to callBuild binds three arguments explicitly, which indicates that, depending on how many arguments are passed to handleField, the Values&& argument of the last call to callBuild receives a mix of the original arguments and the onces that are added while recursing. However, it looks like the only place where handleField is called, exactly three arguments are passed. In that case, the three arguments are exactly those that are bound explicitly, while the Values&& argument receives exactly the arguments that are collected recursively.

If my analysis is correct, and I haven't missed another place where handleField is called with a different count of arguments, then this code is needlessly complex.

Update: After further analysis of the code, understood that all it does is unpack a tuple. I replaced the custom unpacking code with std::apply.

ryanofsky · 2025-02-28T20:42:28Z

After further analysis of the code, understood that all it does is unpack a tuple.

Yep, this code was only written this way to work when Bitcoin core was using c++11. Now it can be much simpler and 36898db seems right. This also looks like a nice change because it is declaring more explicit types (which would have been possible with c++11 too).

hebasto · 2025-03-03T12:16:03Z

Concept ACK.

ryanofsky

Code review ACK 049d931. Thanks for the fixes! This looks good in current form, but if interested in making improvements would suggest:

Including linter error messages in commit messages so it is clear what is motivating the changes.
Getting rid of fun variables in 36898db so fewer lines have to change and the code is simpler.
Using suggested changes to fix lint errors in the generated files instead of disabling linter.

ryanofsky · 2025-03-11T01:06:10Z

CMakeLists.txt

@@ -51,6 +51,7 @@ configure_file(include/mp/config.h.in "${CMAKE_CURRENT_BINARY_DIR}/include/mp/co

 # Generated C++ Capn'Proto schema files
 capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp)
+set_source_files_properties(${MP_PROXY_SRCS} PROPERTIES SKIP_LINTING ON)


In commit "disable linting of generated files" (764128a)

Seems ok to skip linting, but in general I think it would be better if we could run the linters on generated files to be alerted about problems in generated code that we could fix. Last time I tried running linters there were just a few modernize-use-override errors which make sense to disable because generated code can't know if classes it is inheriting from use virtual methods or not. The fix I made at the time was:

diff

--- a/src/ipc/libmultiprocess/src/mp/gen.cpp +++ b/src/ipc/libmultiprocess/src/mp/gen.cpp @@ -252,6 +256,7 @@ static void Generate(kj::StringPtr src_prefix, h << "#pragma GCC diagnostic ignored \"-Wsuggest-override\"\n"; h << "#endif\n"; h << "#endif\n"; + h << "// NOLINTBEGIN(modernize-use-override)\n"; h << "namespace mp {\n"; kj::StringPtr message_namespace; @@ -628,6 +633,7 @@ static void Generate(kj::StringPtr src_prefix, inl << "#endif\n"; h << "} // namespace mp\n"; + h << "// NOLINTEND(modernize-use-override)\n"; h << "#if defined(__GNUC__)\n"; h << "#pragma GCC diagnostic pop\n"; h << "#endif\n";

And I would still recommend that change as an alternative to this commit. Current commit also seems ok though and I always can PR the other change separately.

ryanofsky · 2025-03-11T01:17:30Z

include/mp/proxy-types.h

+    decltype(auto) get() const { return Accessor::get(this->m_struct); }
+
+    bool has() const {
+      if constexpr (Accessor::optional) {


In commit "replace SFINAE trick with if constexpr" (8ed7b63)

Seems like good changes, but is indentation in this part of the code supposed to use 4 spaces instead of 2? Would be good to make this consistent and maybe use clang-format

maybe use clang-format

Would be great if there was a .clang-format file.

re: #165 (comment)

Note: Spacing is still not consistent, but should be fine to reformat later.

include/mp/proxy-types.h

ryanofsky · 2025-03-11T02:40:28Z

include/mp/proxy-types.h

        {
-            callRead<I + 1>(std::forward<Args>(args)..., std::get<I>(m_client_param->m_values));
-        }
+            auto const fun = [&]<typename... Values>(Values&&... values) {


In commit "replace custom tuple unpacking code with std::apply" (36898db)

Note: Values type here is currently unused, so lambda declaration could be simplified to just take an auto&& values parameter. However, keeping Values is nice even though it is unused because it makes ReadParams and BuildResults code more consistent. Also it might be useful to add perfect forwarding for ReadDestUpdate(std::forward<Values>(values)) in the future. That will require other changes to this code however, so better to leave alone for now.

In general there are a lot more simplifications that can be made here now that this code no longer needs to work with c++11. Would be good to follow up in a separate PR.

src/mp/gen.cpp

ryanofsky · 2025-05-12T14:43:27Z

@purpleKarrot Do you want to follow up to review comments with #165 (review)? If not, I can merge this is as-is and make other changes in a new PR, or just cherry-pick changes from here into a new PR. You can let me know what your prefer.

Asking because Sjors reported a new clang-tidy error in bitcoin/bitcoin#31802 (comment) so it looks like some additional fixes may be necessary.

clang-tidy recomments replacing `enable_if` with C++20 `requires`. However, using `if constexpr` results in simpler code here.

clang-tidy recomments replacing `enable_if` with C++20 `requires`. However, the code can be simplified with `std::apply`.

Apply clang-tidy's readability-make-member-function-const fixit.

Apply clang-tidy's modernize-use-ranges fixit.

purpleKarrot · 2025-05-13T17:41:59Z

@ryanofsky, I dropped the commit that disables linting generated files and extended commit messages.

ryanofsky

Code review ACK aa19285

Looks good! Will probably merge shortly. Only changes since last review were dropping the SKIP_LINTING commit (764128a) and now describing the clang-tidy errors that are fixed in commit messages.

Note: without SKIP_LINTING commit probably will want to follow up with code generator diff posted at #165 (comment)

ryanofsky · 2025-05-15T13:30:48Z

include/mp/proxy-types.h

+    decltype(auto) get() const { return Accessor::get(this->m_struct); }
+
+    bool has() const {
+      if constexpr (Accessor::optional) {


re: #165 (comment)

Note: Spacing is still not consistent, but should be fine to reformat later.

src/mp/gen.cpp

a5bff37ad8 clang-tidy: Suppress bitcoin-nontrivial-threadlocal error e922bb8f6a clang-tidy: Fix bugprone-move-forwarding-reference error c1e8c1a028 clang-tidy: Fix bugprone-move-forwarding-reference errors 0d8012f656 Merge bitcoin-core/libmultiprocess#165: clang-tidy: fix warnings introduced in version 19 aa19285303 use ranges transform a78137ca73 make member function const ca3226ec8a replace custom tuple unpacking code with `std::apply` 949fe85fc9 replace SFINAE trick with `if constexpr` git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: a5bff37ad8f5630a738eb1b0bfa60cf01bedbcdf

57a65b8546 clang-tidy: Suppress bitcoin-nontrivial-threadlocal error 3a96cdc18f clang-tidy: Fix bugprone-move-forwarding-reference error c1e8c1a028 clang-tidy: Fix bugprone-move-forwarding-reference errors 0d8012f656 Merge bitcoin-core/libmultiprocess#165: clang-tidy: fix warnings introduced in version 19 aa19285303 use ranges transform a78137ca73 make member function const ca3226ec8a replace custom tuple unpacking code with `std::apply` 949fe85fc9 replace SFINAE trick with `if constexpr` git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 57a65b854664c13c9801788eeaf71bdb3e597c81

…e5a581 27c7e8e5a581 Merge bitcoin-core/libmultiprocess#172: refactor: fix warnings from clang-tidy-20 and bitcoin-tidy 2fe87d016be4 Merge bitcoin-core/libmultiprocess#173: doc: Fix error string typo 57a65b854664 clang-tidy: Suppress bitcoin-nontrivial-threadlocal error 0d8012f656fe Merge bitcoin-core/libmultiprocess#165: clang-tidy: fix warnings introduced in version 19 3a96cdc18f2d clang-tidy: Fix bugprone-move-forwarding-reference error c1e8c1a02864 clang-tidy: Fix bugprone-move-forwarding-reference errors aa19285303ff use ranges transform a78137ca73b8 make member function const ca3226ec8ab7 replace custom tuple unpacking code with `std::apply` 949fe85fc91f replace SFINAE trick with `if constexpr` 44ee4b40b89a doc: Fix error string typo git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 27c7e8e5a581b3c41330e758951251ef11807b11

154af1e Squashed 'src/ipc/libmultiprocess/' changes from 35944ffd23fa..27c7e8e5a581 (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#165 - bitcoin-core/libmultiprocess#173 - bitcoin-core/libmultiprocess#172 These changes are needed to fix CI errors in #31802. The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) ACKs for top commit: Sjors: ACK 9f65654 TheCharlatan: ACK 9f65654 Tree-SHA512: 745789a6fba552aa066f9f69592b16a5277999dbf0413eaed7fe25cd440b78af57615edfce781592873659fda91463bef7c5dac202b80362bd86f6a90ab20d69

…e5a581 27c7e8e5a581 Merge bitcoin-core/libmultiprocess#172: refactor: fix warnings from clang-tidy-20 and bitcoin-tidy 2fe87d016be4 Merge bitcoin-core/libmultiprocess#173: doc: Fix error string typo 57a65b854664 clang-tidy: Suppress bitcoin-nontrivial-threadlocal error 0d8012f656fe Merge bitcoin-core/libmultiprocess#165: clang-tidy: fix warnings introduced in version 19 3a96cdc18f2d clang-tidy: Fix bugprone-move-forwarding-reference error c1e8c1a02864 clang-tidy: Fix bugprone-move-forwarding-reference errors aa19285303ff use ranges transform a78137ca73b8 make member function const ca3226ec8ab7 replace custom tuple unpacking code with `std::apply` 949fe85fc91f replace SFINAE trick with `if constexpr` 44ee4b40b89a doc: Fix error string typo git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 27c7e8e5a581b3c41330e758951251ef11807b11

purpleKarrot commented Feb 28, 2025

View reviewed changes

include/mp/proxy-types.h Outdated Show resolved Hide resolved

purpleKarrot force-pushed the clang-tidy branch from 1713d5b to cc84cda Compare February 28, 2025 10:44

purpleKarrot force-pushed the clang-tidy branch from cc84cda to 049d931 Compare February 28, 2025 19:52

ryanofsky approved these changes Mar 11, 2025

View reviewed changes

ryanofsky mentioned this pull request May 12, 2025

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

Merged

purpleKarrot added 4 commits May 13, 2025 18:47

replace SFINAE trick with if constexpr

949fe85

clang-tidy recomments replacing `enable_if` with C++20 `requires`. However, using `if constexpr` results in simpler code here.

replace custom tuple unpacking code with std::apply

ca3226e

clang-tidy recomments replacing `enable_if` with C++20 `requires`. However, the code can be simplified with `std::apply`.

make member function const

a78137c

Apply clang-tidy's readability-make-member-function-const fixit.

use ranges transform

aa19285

Apply clang-tidy's modernize-use-ranges fixit.

purpleKarrot force-pushed the clang-tidy branch from 049d931 to aa19285 Compare May 13, 2025 16:54

ryanofsky approved these changes May 15, 2025

View reviewed changes

ryanofsky merged commit 0d8012f into bitcoin-core:master May 15, 2025

ryanofsky mentioned this pull request May 15, 2025

refactor: EventLoop locking cleanups + client disconnect exception #160

Merged

ryanofsky mentioned this pull request May 29, 2025

Update libmultiprocess subtree to fix clang-tidy errors bitcoin/bitcoin#32641

Merged

clang-tidy: fix warnings introduced in version 19 #165

clang-tidy: fix warnings introduced in version 19 #165

Uh oh!

Conversation

purpleKarrot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

purpleKarrot commented Feb 28, 2025

Uh oh!

ryanofsky commented Feb 28, 2025

Uh oh!

purpleKarrot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Feb 28, 2025

Uh oh!

hebasto commented Mar 3, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

purpleKarrot May 13, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented May 12, 2025

Uh oh!

purpleKarrot commented May 13, 2025

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

purpleKarrot commented Feb 28, 2025 •

edited

Loading

purpleKarrot commented Feb 28, 2025 •

edited

Loading

ryanofsky left a comment •

edited

Loading