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

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

Should fix #153.

purpleKarrot · 2025-02-28T09:57:13Z

include/mp/proxy-types.h

+    template<typename A = Accessor> void setHas() const requires(A::optional) { return A::setHas(m_struct); }
+    template<typename A = Accessor> void setHas() const requires(!A::optional) { }
+    template<typename A = Accessor> void setWant() const requires(A::requested) { return A::setWant(m_struct); }
+    template<typename A = Accessor> void setWant() const requires(!A::requested) { }


Actually, they all should use if constexpr.

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

ryanofsky · 2025-03-11T02:33:27Z

include/mp/proxy-types.h

-                Make<StructField, Accessor>(params), std::forward<Values>(values)...);
-            MaybeSetWant(
-                ParamList(), Priority<1>(), std::forward<Values>(values)..., Make<StructField, Accessor>(params));
+            auto const fun = [&]<typename... Values>(Values&&... values) {


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

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.

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.

ryanofsky · 2025-03-11T02:45:35Z

src/mp/gen.cpp

@@ -215,7 +215,7 @@ static void Generate(kj::StringPtr src_prefix,
    cpp_types << "namespace mp {\n";

    std::string guard = output_path;
-    std::transform(guard.begin(), guard.end(), guard.begin(), [](unsigned char c) -> unsigned char {
+    std::ranges::transform(guard, guard.begin(), [](unsigned char c) -> unsigned char {


In commit "use ranges transform" (049d931)

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.

disable linting of generated files

764128a

purpleKarrot commented Feb 28, 2025

View reviewed changes

replace SFINAE trick with if constexpr

8ed7b63

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

replace custom tuple unpacking code with std::apply

36898db

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

purpleKarrot added 2 commits February 28, 2025 20:52

make member function const

9a01a35

use ranges transform

049d931

ryanofsky approved these changes Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

purpleKarrot commented Feb 28, 2025 •

edited

Loading

purpleKarrot Feb 28, 2025

purpleKarrot commented Feb 28, 2025

ryanofsky commented Feb 28, 2025

purpleKarrot commented Feb 28, 2025 •

edited

Loading

ryanofsky commented Feb 28, 2025

hebasto commented Mar 3, 2025

ryanofsky left a comment

ryanofsky Mar 11, 2025

ryanofsky Mar 11, 2025

ryanofsky Mar 11, 2025

ryanofsky Mar 11, 2025

ryanofsky Mar 11, 2025

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

Are you sure you want to change the base?

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

Conversation

purpleKarrot commented Feb 28, 2025 • edited Loading

purpleKarrot Feb 28, 2025

Choose a reason for hiding this comment

purpleKarrot commented Feb 28, 2025

ryanofsky commented Feb 28, 2025

purpleKarrot commented Feb 28, 2025 • edited Loading

ryanofsky commented Feb 28, 2025

hebasto commented Mar 3, 2025

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

ryanofsky Mar 11, 2025

Choose a reason for hiding this comment

purpleKarrot commented Feb 28, 2025 •

edited

Loading

purpleKarrot commented Feb 28, 2025 •

edited

Loading