clang-tidy: fix warnings introduced in version 19

purpleKarrot commented at 9:08 am on February 28, 2025: contributor

Should fix #153.

in include/mp/proxy-types.h:52 in 1713d5b3d0 outdated

56-    template<typename A = Accessor> auto setWant() const -> std::enable_if_t<A::requested> { return A::setWant(m_struct); }
57-    template<typename A = Accessor> auto setWant() const -> std::enable_if_t<!A::requested> { }
58+    template<typename A = Accessor> void setHas() const requires(A::optional) { return A::setHas(m_struct); }
59+    template<typename A = Accessor> void setHas() const requires(!A::optional) { }
60+    template<typename A = Accessor> void setWant() const requires(A::requested) { return A::setWant(m_struct); }
61+    template<typename A = Accessor> void setWant() const requires(!A::requested) { }

purpleKarrot commented at 9:57 am on February 28, 2025:

Actually, they all should use if constexpr.

purpleKarrot force-pushed on Feb 28, 2025

purpleKarrot commented at 11:25 am on February 28, 2025: contributor

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

ryanofsky commented at 2:25 pm on February 28, 2025: collaborator

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 commented at 4:14 pm on February 28, 2025: contributor

@ryanofsky, I am referring to those changes: https://github.com/chaincodelabs/libmultiprocess/pull/165/commits/0dd3a4e4946de1dda6b5814bbbca1ba23e4b59b0

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

0// TODO Possible optimization to speed up compile time:
1// https://stackoverflow.com/a/7858971 Using enable_if below to check
2// position when unpacking tuple might be slower than pattern matching
3// 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.

purpleKarrot force-pushed on Feb 28, 2025

ryanofsky commented at 8:42 pm on February 28, 2025: collaborator

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 36898dbfc72c9748f54bf79f18d1ad6485f48920 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 commented at 12:16 pm on March 3, 2025: member

Concept ACK.

in CMakeLists.txt:54 in 764128a1f7 outdated

50@@ -51,6 +51,7 @@ configure_file(include/mp/config.h.in "${CMAKE_CURRENT_BINARY_DIR}/include/mp/co
51 
52 # Generated C++ Capn'Proto schema files
53 capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp)
54+set_source_files_properties(${MP_PROXY_SRCS} PROPERTIES SKIP_LINTING ON)

ryanofsky commented at 1:06 am on March 11, 2025:

In commit “disable linting of generated files” (764128a1f7bf84fa7713d1c80a1f6b647dfe8a1e)

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:

 0--- a/src/ipc/libmultiprocess/src/mp/gen.cpp
 1+++ b/src/ipc/libmultiprocess/src/mp/gen.cpp
 2@@ -252,6 +256,7 @@ static void Generate(kj::StringPtr src_prefix,
 3     h << "#pragma GCC diagnostic ignored \"-Wsuggest-override\"\n";
 4     h << "#endif\n";
 5     h << "#endif\n";
 6+    h << "// NOLINTBEGIN(modernize-use-override)\n";
 7     h << "namespace mp {\n";
 8 
 9     kj::StringPtr message_namespace;
10@@ -628,6 +633,7 @@ static void Generate(kj::StringPtr src_prefix,
11     inl << "#endif\n";
12 
13     h << "} // namespace mp\n";
14+    h << "// NOLINTEND(modernize-use-override)\n";
15     h << "#if defined(__GNUC__)\n";
16     h << "#pragma GCC diagnostic pop\n";
17     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.

in include/mp/proxy-types.h:43 in 8ed7b63640 outdated

52-    template<typename A = Accessor> auto setWant() const -> std::enable_if_t<!A::requested> { }
53-    // clang-format on
54+    decltype(auto) get() const { return Accessor::get(this->m_struct); }
55+
56+    bool has() const {
57+      if constexpr (Accessor::optional) {

ryanofsky commented at 1:17 am on March 11, 2025:

In commit “replace SFINAE trick with if constexpr” (8ed7b6364033e33088790f01251d3b715ae115c1)

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

purpleKarrot commented at 4:30 pm on May 13, 2025:

maybe use clang-format

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

ryanofsky commented at 1:30 pm on May 15, 2025:

re: #165 (review)

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

in include/mp/proxy-types.h:395 in 36898dbfc7 outdated

413         {
414-            MaybeBuildField(std::integral_constant<bool, Accessor::in>(), ParamList(), invoke_context,
415-                Make<StructField, Accessor>(params), std::forward<Values>(values)...);
416-            MaybeSetWant(
417-                ParamList(), Priority<1>(), std::forward<Values>(values)..., Make<StructField, Accessor>(params));
418+            auto const fun = [&]<typename... Values>(Values&&... values) {

ryanofsky commented at 2:33 am on March 11, 2025:

In commit “replace custom tuple unpacking code with std::apply” (36898dbfc72c9748f54bf79f18d1ad6485f48920)

I think this code might be more straightforward and easier to review if this got rid of the fun variables and just passed the lambda directly as the first argument to std::apply. That way the indentation of the code inside the lambda would not have to change and the diff would be smaller and more obvious, and the resulting code would also be shorter.

purpleKarrot commented at 4:45 pm on May 13, 2025:

That way the indentation of the code inside the lambda would not have to change

It does. Using the lambda directly has no effect on the number of { and hence the indentation. Also, running git clang-format after trying that completely borked the formatting due to no .clang-format.

I prefer passing lambda expression to functions directly iff the expression is small and it is the last function argument. Here, the lambda expression is complex and it is not the last function argument. Giving it a name results in longer, but more readable code.

in include/mp/proxy-types.h:414 in 36898dbfc7 outdated

419+        template <typename Results, typename... Params>
420+        void handleField(ClientInvokeContext& invoke_context, Results& results, TypeList<Params...>)
421         {
422-            callRead<I + 1>(std::forward<Args>(args)..., std::get<I>(m_client_param->m_values));
423-        }
424+            auto const fun = [&]<typename... Values>(Values&&... values) {

ryanofsky commented at 2:40 am on March 11, 2025:

In commit “replace custom tuple unpacking code with std::apply” (36898dbfc72c9748f54bf79f18d1ad6485f48920)

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.

in src/mp/gen.cpp:218 in 049d93113f outdated

214@@ -215,7 +215,7 @@ static void Generate(kj::StringPtr src_prefix,
215     cpp_types << "namespace mp {\n";
216 
217     std::string guard = output_path;
218-    std::transform(guard.begin(), guard.end(), guard.begin(), [](unsigned char c) -> unsigned char {
219+    std::ranges::transform(guard, guard.begin(), [](unsigned char c) -> unsigned char {

ryanofsky commented at 2:45 am on March 11, 2025:

In commit “use ranges transform” (049d93113f27aca50381028e082c5538c0b1c574)

It’s not really clear why this new code is better here. Would be good to have some explanation or note what the warning is in the commit message.

ryanofsky commented at 1:36 pm on May 15, 2025:

re: #165 (review)

Thanks for the update. From https://cirrus-ci.com/task/6187773452877824?logs=ci#L4758, it seems like clang-tidy error triggering this just looks like: warning: use a ranges version of this algorithm [modernize-use-ranges]

ryanofsky approved

ryanofsky commented at 2:47 am on March 11, 2025: collaborator

Code review ACK 049d93113f27aca50381028e082c5538c0b1c574. 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 36898dbfc72c9748f54bf79f18d1ad6485f48920 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 commented at 2:43 pm on May 12, 2025: collaborator

@purpleKarrot Do you want to follow up to review comments with #165#pullrequestreview-2672482311? 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 https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858241771 so it looks like some additional fixes may be necessary.

replace SFINAE trick with `if constexpr`

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

949fe85fc9

replace custom tuple unpacking code with `std::apply`

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

ca3226ec8a

make member function const

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

a78137ca73

use ranges transform

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

aa19285303

purpleKarrot force-pushed on May 13, 2025

purpleKarrot commented at 5:41 pm on May 13, 2025: contributor

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

ryanofsky approved

ryanofsky commented at 1:40 pm on May 15, 2025: collaborator

Code review ACK aa19285303fff8db9bf8d53491d8a6e6e1617397

Looks good! Will probably merge shortly. Only changes since last review were dropping the SKIP_LINTING commit (764128a1f7bf84fa7713d1c80a1f6b647dfe8a1e) 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 (review)

ryanofsky merged this on May 15, 2025

ryanofsky closed this on May 15, 2025

Sjors referenced this in commit 62564a5b1c on May 16, 2025

Sjors referenced this in commit 5d45c31e9b on May 28, 2025

Sjors referenced this in commit 0debfd6c7f on May 28, 2025

Sjors referenced this in commit 88fe80ab1b on May 28, 2025

ryanofsky referenced this in commit 154af1eea1 on May 29, 2025

fanquake referenced this in commit 9393aeeca4 on May 30, 2025

saikiran57 referenced this in commit 8066d9c3d2 on Jul 28, 2025

janus referenced this in commit fcaf9ff8af on Sep 14, 2025

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