Address `clang-tidy-19` warnings

hebasto commented at 11:48 pm on January 31, 2025: member

This PR continues the work from https://github.com/chaincodelabs/libmultiprocess/pull/83 and addresses new warnings for clang-tidy-19:

0$ cmake -B build -DCMAKE_CXX_COMPILER=clang++-19 -DCMAKE_CXX_CLANG_TIDY=clang-tidy-19
1$ cmake --build build --target all mptests mpexamples

All generated files, including those produced by the target_capnp_sources() function, are no longer subject to any CMake’s linting process (CMake 3.27 or later is required; however, this should be a reasonable expectation for environments where linting is performed).

cmake: Skip linting for generated sources a692038014

clang-tidy: Disable `misc-use-anonymous-namespace` check

The benefit of this check is questionable, so it has been disabled.
See: https://clang.llvm.org/extra/clang-tidy/checks/misc/use-anonymous-namespace.html

3c07ab89af

clang-tidy: Disable `performance-avoid-endl` check

See: https://clang.llvm.org/extra/clang-tidy/checks/performance/avoid-endl.html

d6523b60a0

clang-tidy: Fix `bugprone-crtp-constructor-accessibility` check

See: https://clang.llvm.org/extra/clang-tidy/checks/bugprone/empty-catch.html

1cc0bf573f

clang-tidy: Suppress `bugprone-empty-catch` check warning

See: https://clang.llvm.org/extra/clang-tidy/checks/bugprone/empty-catch.html

5a15744d4a

clang-tidy: Fix `modernize-type-traits` check

See: https://clang.llvm.org/extra/clang-tidy/checks/modernize/type-traits.html

f983b09a31

clang-tidy: Fix `misc-const-correctness` check

See: https://clang.llvm.org/extra/clang-tidy/checks/misc/const-correctness.html

06806fe90c

clang-tidy: Fix `misc-include-cleaner` check

See: https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html

90cb5a9017

clang-tidy: Fix `misc-use-internal-linkage` check

See: https://clang.llvm.org/extra/clang-tidy/checks/misc/use-internal-linkage.html

2ad6c17574

clang-tidy: Fix `performance-unnecessary-value-param` check

See: https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-value-param.html

584f3e2537

clang-tidy: Fix `readability-avoid-nested-conditional-operator` check

See: https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-nested-conditional-operator.html

fa265a9d9f

clang-tidy: Fix `readability-avoid-return-with-void-value` check

See: https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-return-with-void-value.html

f159c75e0e

clang-tidy: Fix `readability-container-size-empty` check

See: https://clang.llvm.org/extra/clang-tidy/checks/readability/container-size-empty.html

3418ad20c1

clang-tidy: Suppress `performance-enum-size` check warning

See: https://clang.llvm.org/extra/clang-tidy/checks/performance/enum-size.html

19d7537cb5

purpleKarrot commented at 1:54 pm on February 1, 2025: contributor

What is the rationale for disabling some of the checks? Are fixes postponed for follow up work or is there another reason to keep them?

hebasto commented at 2:05 pm on February 1, 2025: member

What is the rationale for disabling some of the checks? Are fixes postponed for follow up work or is there another reason to keep them?

The performance-avoid-endl check affects only code in examples. I see no reason to churn it solely for a marginal performance gain.

The misc-use-anonymous-namespace check is questionable on its own. There have been multiple long discussions in the Bitcoin Core repo, our IRC channel, and on C++ forums in general. I’d prefer not to enforce a particular style on developers. That said, I personally prefer anonymous namespaces.

purpleKarrot commented at 2:31 pm on February 1, 2025: contributor

ACK

The performance-avoid-endl check affects only code in examples. I see no reason to churn it solely for a marginal performance gain.

It is not only about performance. It also sends a signal to readers that std::endl is not welcome.

in include/mp/proxy-types.h:319 in 19d7537cb5

313@@ -314,6 +314,9 @@ struct IterateFieldsHelper
314     {
315         static_cast<Derived*>(this)->handleField(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), ParamList());
316     }
317+private:
318+    IterateFieldsHelper() = default;
319+    friend Derived;

vasild commented at 3:05 pm on February 3, 2025:

Reverting commit

1cc0bf573fdd23ce190b58c20e88bc21f943e3bb clang-tidy: Fix bugprone-crtp-constructor-accessibility check

does not reveal any warnings, as if that commit is not needed?

hebasto commented at 4:00 pm on February 3, 2025:

With the reverted commit on Ubuntu 24.10:

0$ cmake -B build -DCMAKE_CXX_COMPILER=clang++-19 -DCMAKE_CXX_CLANG_TIDY=clang-tidy-19
1$ cmake --build build --target all mptests mpexamples
2[48/59] Building CXX object CMakeFiles/multiprocess.dir/src/mp/proxy.cpp.o
3/home/hebasto/git/libmultiprocess/include/mp/proxy-types.h:301:8: warning: the implicit default constructor of the CRTP is publicly accessible; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]
4  301 | struct IterateFieldsHelper
5      |        ^
6  302 | {
7      |  
8[59/59] Linking CXX executable test/mptest

Here are Clang details:

 0$ clang++-19 -v
 1Ubuntu clang version 19.1.1 (1ubuntu1)
 2Target: x86_64-pc-linux-gnu
 3Thread model: posix
 4InstalledDir: /usr/lib/llvm-19/bin
 5Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/11
 6Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/12
 7Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/13
 8Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/14
 9Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/14
10Candidate multilib: .;@m64
11Selected multilib: .;@m64

vasild commented at 9:43 am on February 4, 2025:

Thanks for posting the exact commands (I should have posted mine in the first place). It turns out that if the build directory is outside of the source directory, then clang-tidy does not find its config file. This is strange because it should find it in the parent directory of the source file. Anyway, I guess you can reproduce this:

0cmake -B /tmp/build -DCMAKE_CXX_COMPILER=clang++-19 -DCMAKE_CXX_CLANG_TIDY=clang-tidy-19
1cmake --build /tmp/build --target all mptests mpexamples

and observe no warnings from clang-tidy.

This resolves the problem:

 0--- i/CMakeLists.txt
 1+++ w/CMakeLists.txt
 2@@ -18,13 +18,13 @@ find_package(Threads REQUIRED)
 3 option(Libmultiprocess_ENABLE_CLANG_TIDY "Run clang-tidy with the compiler." OFF)
 4 if(Libmultiprocess_ENABLE_CLANG_TIDY)
 5   find_program(CLANG_TIDY_EXECUTABLE NAMES clang-tidy)
 6   if(NOT CLANG_TIDY_EXECUTABLE)
 7     message(FATAL_ERROR "Libmultiprocess_ENABLE_CLANG_TIDY is ON but clang-tidy is not found.")
 8   endif()
 9-  set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}")
10+  set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}" --config-file=${CMAKE_SOURCE_DIR}/.clang-tidy)
11 endif()
12 
13 include("cmake/compat_config.cmake")
14 include("cmake/pthread_checks.cmake")
15 include(GNUInstallDirs)

if -DLibmultiprocess_ENABLE_CLANG_TIDY=ON is to be used. Or if CMAKE_CXX_CLANG_TIDY is to be set directly on the command line, then it should be

0cmake -B /tmp/build -DCMAKE_CXX_COMPILER=clang++19 -DCMAKE_CXX_CLANG_TIDY="clang-tidy19;--config-file=/path_to_source/libmultiprocess/.clang-tidy"

“config file not found when out of source” is out of the scope of this PR, so I guess this issue can be marked as resolved. I will be using a subdirectory of the source to test this PR.

in include/mp/util.h:161 in 19d7537cb5

156@@ -157,7 +157,7 @@ struct DestructorCatcher
157     {
158     }
159     ~DestructorCatcher() noexcept try {
160-    } catch (const kj::Exception& e) {
161+    } catch (const kj::Exception& e) { // NOLINT(bugprone-empty-catch)
162     }

vasild commented at 3:49 pm on February 3, 2025:

From https://clang.llvm.org/extra/clang-tidy/checks/bugprone/empty-catch.html

To avoid these issues, developers should always handle exceptions properly

This warning looks legit. I wonder if it can be resolved instead of suppressed.

I am unable to reproduce it, so I am probably doing something wrong. But this should resolve it, suggested from https://github.com/capnproto/capnproto/issues/553#issuecomment-328554603:

 0--- i/include/mp/util.h
 1+++ w/include/mp/util.h
 2@@ -6,12 +6,13 @@
 3 #define MP_UTIL_H
 4 
 5 #include <capnp/schema.h>
 6 #include <cstddef>
 7 #include <functional>
 8 #include <future>
 9+#include <kj/debug.h>
10 #include <kj/common.h>
11 #include <kj/exception.h>
12 #include <kj/string-tree.h>
13 #include <memory>
14 #include <string.h>
15 #include <string>
16@@ -154,13 +155,14 @@ struct DestructorCatcher
17     T value;
18     template <typename... Params>
19     DestructorCatcher(Params&&... params) : value(kj::fwd<Params>(params)...)
20     {
21     }
22     ~DestructorCatcher() noexcept try {
23-    } catch (const kj::Exception& e) { // NOLINT(bugprone-empty-catch)
24+    } catch (const kj::Exception& e) {
25+        KJ_LOG(ERROR, e);
26     }
27 };
28 
29 //! Wrapper around callback function for compatibility with std::async.
30 //!
31 //! std::async requires callbacks to be copyable and requires noexcept

Further, unrelated to this PR, the DestructorCatcher does not seem to work in the first place:

 0class A
 1{
 2public:
 3    ~A()
 4    {
 5        throw std::runtime_error{"aaa"};
 6    }
 7};
 8
 9template <typename T>
10struct DestructorCatcher
11{
12    T value;
13    template <typename... Params>
14        DestructorCatcher(Params&&... params) : value(std::forward<Params>(params)...)
15    {
16    }
17    ~DestructorCatcher() noexcept try {
18    } catch (const std::runtime_error& e) {
19        // Does not work with or without a return.
20        //return;
21    }
22};
23
24int main(int, char**)
25{
26    try {
27        std::shared_ptr<DestructorCatcher<A>> p{std::make_shared<DestructorCatcher<A>>()};
28    } catch (const std::runtime_error& e) {
29        std::cout << "in main: " << e.what() << "\n";
30    }
31    return 0;
32}

0  * frame [#0](/bitcoin-core-multiprocess/0/): 0x0000000825ade18a libc.so.7`__sys_thr_kill at thr_kill.S:4
1    frame [#1](/bitcoin-core-multiprocess/1/): 0x0000000825a575a4 libc.so.7`__raise(s=6) at raise.c:50:10
2    frame [#2](/bitcoin-core-multiprocess/2/): 0x0000000825b0ab29 libc.so.7`abort at abort.c:64:8
3    frame [#3](/bitcoin-core-multiprocess/3/): 0x0000000000203c0e t`__clang_call_terminate + 14
4    frame [#4](/bitcoin-core-multiprocess/4/): 0x0000000000204e09 t`A::~A(this=0x00000e1f0581a038) at t.cc:184:15
5    frame [#5](/bitcoin-core-multiprocess/5/): 0x0000000000204db5 t`DestructorCatcher<A>::~DestructorCatcher(this=0x00000e1f0581a038) at t.cc:197:5
6    frame [#7](/bitcoin-core-multiprocess/7/): 0x0000000000204d59 t`void std::__1::allocator_traits<std::__1::allocator<DestructorCatcher<A>>>::destroy[abi:de180100]<DestructorCatcher<A>, void, void>((null)=0x00000008205e8697, __p=0x00000e1f0581a038) at allocator_traits.h:316:5
7    frame [#12](/bitcoin-core-multiprocess/12/): 0x0000000000203bf8 t`std::__1::shared_ptr<DestructorCatcher<A>>::~shared_ptr[abi:de180100](this=0x00000008205e8770) at shared_ptr.h:648:17
8    frame [#13](/bitcoin-core-multiprocess/13/): 0x0000000000203aaa t`main((null)=1, (null)=0x00000008205e8818) at t.cc:206:5

ryanofsky commented at 5:25 pm on February 3, 2025:

re: https://github.com/chaincodelabs/libmultiprocess/pull/144#discussion_r1939618932

Good catch. The workaround suggested https://github.com/capnproto/capnproto/issues/553#issuecomment-328554603 is not implemented correctly here. The derived constructor is trying to catch exceptions from a base constructor which will not even run until after it exits. To be implemented correctly it would have to add T as a member instead of deriving from it, and either set T to null or make it a std::optional member to make sure its destruct wont throw after DestructorCatcher exits.

But I think a better fix for this issue is to change EventLoop::post method to use kj::Function instead of std::function so DestructorCatcher and AsyncCallable classes can just be deleted.

vasild commented at 11:24 am on February 4, 2025:

Now that I can reproduce, I confirm that the above patch which adds KJ_LOG() silences the warning and I think that is better than suppressing. @ryanofsky, I am confused. Do you mean “destructor” instead of “constructor”? Also, there is no inheritance here, so there is no derived constructor. I tried to change T value; in the standalone prog above to std::unique_ptr<T> value;, std::optional<T> value; or to T* value; and changed the ~DestructorCatcher() to:

0      ~DestructorCatcher() noexcept
1      {
2          try {
3              delete value;
4          } catch (const std::runtime_error& e) {
5              std::cout << e.what() << "\n";
6          }
7      }

but it keeps crashing.

But I think a better fix for this issue is to change EventLoop::post method to use kj::Function

:+1:

vasild commented at 1:32 pm on February 4, 2025:

… change EventLoop::post method to use kj::Function …

Quick attempt at doing that, I am not sure how to resolve the compilation error in sync():

  0diff --git i/include/mp/proxy-io.h w/include/mp/proxy-io.h
  1index 4430a42..14d5906 100644
  2--- i/include/mp/proxy-io.h
  3+++ w/include/mp/proxy-io.h
  4@@ -8,12 +8,13 @@
  5 #include <mp/proxy.h>
  6 #include <mp/util.h>
  7 
  8 #include <mp/proxy.capnp.h>
  9 
 10 #include <capnp/rpc-twoparty.h>
 11+#include <kj/function.h>
 12 
 13 #include <assert.h>
 14 #include <functional>
 15 #include <optional>
 16 #include <map>
 17 #include <memory>
 18@@ -141,20 +142,22 @@ public:
 19     //! called once from the m_thread_id thread. This will block until
 20     //! the m_num_clients reference count is 0.
 21     void loop();
 22 
 23     //! Run function on event loop thread. Does not return until function completes.
 24     //! Must be called while the loop() function is active.
 25-    void post(const std::function<void()>& fn);
 26+    void post(kj::Function<void()>& fn);
 27 
 28     //! Wrapper around EventLoop::post that takes advantage of the
 29     //! fact that callable will not go out of scope to avoid requirement that it
 30     //! be copyable.
 31     template <typename Callable>
 32     void sync(Callable&& callable)
 33     {
 34+        // XXX
 35+        // error: no viable conversion from 'reference_wrapper<(lambda at libmultiprocess/include/mp/proxy-io.h:427:43)>' to 'kj::Function<void ()>'
 36         return post(std::ref(callable));
 37     }
 38 
 39     //! Start asynchronous worker thread if necessary. This is only done if
 40     //! there are ProxyServerBase::m_impl objects that need to be destroyed
 41     //! asynchronously, without tying up the event loop thread. This can happen
 42@@ -192,13 +195,13 @@ public:
 43 
 44     //! Handle of an async worker thread. Joined on destruction. Unset if async
 45     //! method has not been called.
 46     std::thread m_async_thread;
 47 
 48     //! Callback function to run on event loop thread during post() or sync() call.
 49-    const std::function<void()>* m_post_fn = nullptr;
 50+    kj::Function<void()>* m_post_fn = nullptr;
 51 
 52     //! Callback functions to run on async thread.
 53     CleanupList m_async_fns;
 54 
 55     //! Pipe read handle used to wake up the event loop thread.
 56     int m_wait_fd = -1;
 57diff --git i/include/mp/type-context.h w/include/mp/type-context.h
 58index 7c12afe..40d7cc1 100644
 59--- i/include/mp/type-context.h
 60+++ w/include/mp/type-context.h
 61@@ -61,13 +61,13 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
 62 {
 63     const auto& params = server_context.call_context.getParams();
 64     Context::Reader context_arg = Accessor::get(params);
 65     auto future = kj::newPromiseAndFulfiller<typename ServerContext::CallContext>();
 66     auto& server = server_context.proxy_server;
 67     int req = server_context.req;
 68-    auto invoke = MakeAsyncCallable(
 69+    auto invoke =
 70         [fulfiller = kj::mv(future.fulfiller),
 71          call_context = kj::mv(server_context.call_context), &server, req, fn, args...]() mutable {
 72                 const auto& params = call_context.getParams();
 73                 Context::Reader context_arg = Accessor::get(params);
 74                 ServerContext server_context{server, call_context, req};
 75                 bool disconnected{false};
 76@@ -140,13 +140,13 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
 77                 {
 78                     server.m_context.connection->m_loop.sync([&]() {
 79                         auto fulfiller_dispose = kj::mv(fulfiller);
 80                         fulfiller_dispose->reject(kj::mv(*exception));
 81                     });
 82                 }
 83-            });
 84+            };
 85 
 86     // Lookup Thread object specified by the client. The specified thread should
 87     // be a local Thread::Server object, but it needs to be looked up
 88     // asynchronously with getLocalServer().
 89     auto thread_client = context_arg.getThread();
 90     return server.m_context.connection->m_threads.getLocalServer(thread_client)
 91diff --git i/include/mp/util.h w/include/mp/util.h
 92index 0569c44..4fdfd4e 100644
 93--- i/include/mp/util.h
 94+++ w/include/mp/util.h
 95@@ -6,12 +6,13 @@
 96 #define MP_UTIL_H
 97 
 98 #include <capnp/schema.h>
 99 #include <cstddef>
100 #include <functional>
101 #include <future>
102+#include <kj/debug.h>
103 #include <kj/common.h>
104 #include <kj/exception.h>
105 #include <kj/string-tree.h>
106 #include <memory>
107 #include <string.h>
108 #include <string>
109@@ -143,52 +144,12 @@ template <typename Lock, typename Callback>
110 void Unlock(Lock& lock, Callback&& callback)
111 {
112     UnlockGuard<Lock> unlock(lock); // NOLINT(misc-const-correctness)
113     callback();
114 }
115 
116-//! Needed for libc++/macOS compatibility. Lets code work with shared_ptr nothrow declaration
117-//! https://github.com/capnproto/capnproto/issues/553#issuecomment-328554603
118-template <typename T>
119-struct DestructorCatcher
120-{
121-    T value;
122-    template <typename... Params>
123-    DestructorCatcher(Params&&... params) : value(kj::fwd<Params>(params)...)
124-    {
125-    }
126-    ~DestructorCatcher() noexcept try {
127-    } catch (const kj::Exception& e) { // NOLINT(bugprone-empty-catch)
128-    }
129-};
130-
131-//! Wrapper around callback function for compatibility with std::async.
132-//!
133-//! std::async requires callbacks to be copyable and requires noexcept
134-//! destructors, but this doesn't work well with kj types which are generally
135-//! move-only and not noexcept.
136-template <typename Callable>
137-struct AsyncCallable
138-{
139-    AsyncCallable(Callable&& callable) : m_callable(std::make_shared<DestructorCatcher<Callable>>(std::move(callable)))
140-    {
141-    }
142-    AsyncCallable(const AsyncCallable&) = default;
143-    AsyncCallable(AsyncCallable&&) = default;
144-    ~AsyncCallable() noexcept = default;
145-    ResultOf<Callable> operator()() const { return (m_callable->value)(); }
146-    mutable std::shared_ptr<DestructorCatcher<Callable>> m_callable;
147-};
148-
149-//! Construct AsyncCallable object.
150-template <typename Callable>
151-AsyncCallable<std::remove_reference_t<Callable>> MakeAsyncCallable(Callable&& callable)
152-{
153-    return std::move(callable);
154-}
155-
156 //! Format current thread name as "{exe_name}-{$pid}/{thread_name}-{$tid}".
157 std::string ThreadName(const char* exe_name);
158 
159 //! Escape binary string for use in log so it doesn't trigger unicode decode
160 //! errors in python unit tests.
161 std::string LogEscape(const kj::StringTree& string);
162diff --git i/src/mp/proxy.cpp w/src/mp/proxy.cpp
163index ca094e3..5b2fdf6 100644
164--- i/src/mp/proxy.cpp
165+++ w/src/mp/proxy.cpp
166@@ -19,12 +19,13 @@
167 #include <future>
168 #include <kj/async-io.h>
169 #include <kj/async.h>
170 #include <kj/common.h>
171 #include <kj/debug.h>
172 #include <kj/exception.h>
173+#include <kj/function.h>
174 #include <kj/memory.h>
175 #include <map>
176 #include <memory>
177 #include <mutex>
178 #include <stddef.h>
179 #include <stdexcept>
180@@ -215,13 +216,13 @@ void EventLoop::loop()
181     KJ_SYSCALL(::close(post_fd));
182     std::unique_lock<std::mutex> lock(m_mutex); // NOLINT(misc-const-correctness)
183     m_wait_fd = -1;
184     m_post_fd = -1;
185 }
186 
187-void EventLoop::post(const std::function<void()>& fn)
188+void EventLoop::post(kj::Function<void()>& fn)
189 {
190     if (std::this_thread::get_id() == m_thread_id) {
191         fn();
192         return;
193     }
194     std::unique_lock<std::mutex> lock(m_mutex);

ryanofsky commented at 1:48 pm on February 4, 2025:

Thanks for doing ahead with this!

   // error: no viable conversion from 'reference_wrapper<(lambda at libmultiprocess/include/mp/proxy-io.h:427:43)>' to 'kj::Function<void ()>'
   return post(std::ref(callable));

This is just what comes to mind but you might be able to replace std::ref with kj::mv. Intent of std::ref is just to avoid a copy.

ryanofsky commented at 1:54 pm on February 4, 2025:

Now that I can reproduce, I confirm that the above patch which adds KJ_LOG() silences the warning and I think that is better than suppressing.

I’d slightly prefer to explicitly suppress the error so we can know to revisit and fix this later than to add KJ_LOG in code we know is broken and will never run.

@ryanofsky, I am confused. Do you mean “destructor” instead of “constructor”? Also, there is no inheritance here, so there is no derived constructor.

Ooops, I’m not sure what I was looking at when I saw inheritance. And I did mean destructor.

I’m not sure about what was causing crashes you were seeing, but I if we wanted to implement suggested workaround correctly, easiest way would probably be to make member std::optional<T> value and call value.reset() in the try block.

vasild commented at 3:50 pm on February 4, 2025:

I’m not sure about what was causing crashes you were seeing, but I if we wanted to implement suggested workaround correctly, easiest way would probably be to make member std::optional value and call value.reset() in the try block.

That does not work. Probably for the same reason DestructorCatcher is needed in the first place - shared_ptr, unique_ptr, optional do not like containing objects whose destructors throw. It crashes with a plain pointer and delete as well.

Try it yourself: gotbolt

ryanofsky commented at 4:45 pm on February 4, 2025:

Interesting. In that case could maybe an appealing alternative could be declare T in inside an anonymous union and expliict placement-new to call constructor, and explicit call to its destructor in a try/catch. An approach like that might be able to work if can’t get rid of asynccallable class, though I’m still hoping we could get rid of it using kj::Function

ryanofsky commented at 5:29 am on February 10, 2025:

re: https://github.com/chaincodelabs/libmultiprocess/pull/144#discussion_r1941178169

[patch] Remove DestructorCatcher and AsyncCallable

This patch is now part of #160

vasild commented at 3:51 pm on February 3, 2025: contributor

I confirm that with this branch there are no warnings with clang 18, 19 and 20.

Reviewed to (including): 5a15744 clang-tidy: Suppress bugprone-empty-catch check warning

in cmake/TargetCapnpSources.cmake:82 in a692038014 outdated

84+      COMMAND Libmultiprocess::mpgen ${CMAKE_CURRENT_SOURCE_DIR} ${include_prefix} ${CMAKE_CURRENT_SOURCE_DIR}/${capnp_file} ${TCS_IMPORT_PATHS} ${MP_INCLUDE_DIR}
85+      DEPENDS ${capnp_file}
86+      VERBATIM
87+    )
88+    target_sources(${target} PRIVATE ${generated_sources})
89+    set_source_files_properties(${generated_sources} PROPERTIES SKIP_LINTING ON)

ryanofsky commented at 5:01 pm on February 3, 2025:

In commit “cmake: Skip linting for generated sources” (a6920380144a96bf0b127c0ae2d49c86deb826b1)

I think my preference would be to not skip linting for generated files, and I implemented changes locally to actually fix the lint errors, so I think it would be slightly better to drop this commit to avoid having to undo this later. This commit also conflicts with #142 so that could be another reason to drop it. Ok to keep though if you prefer. Just pointing out that this may change later.

in include/mp/proxy-io.h:250 in 06806fe90c outdated

246@@ -247,7 +247,7 @@ struct Waiter
247     template <typename Fn>
248     void post(Fn&& fn)
249     {
250-        std::unique_lock<std::mutex> lock(m_mutex);
251+        std::unique_lock<std::mutex> lock(m_mutex); // NOLINT(misc-const-correctness)

ryanofsky commented at 5:28 pm on February 3, 2025:

In commit “clang-tidy: Fix misc-const-correctness check” (06806fe90ce1f565f19432d13bc7635dd7e6a9f4)

Maybe just declare these const? It seems more stable than adding NOLINT. Especially because if any an unlock call is made to any of these locks later the lint errors would go away but nolint comments would still be there.

ryanofsky commented at 4:14 am on February 5, 2025:

re: https://github.com/chaincodelabs/libmultiprocess/pull/144#discussion_r1939769026

This is done in #152

in example/calculator.cpp:8 in 90cb5a9017 outdated

4@@ -5,13 +5,15 @@
5 #include <calculator.h>
6 #include <fstream>
7 #include <init.capnp.h>
8-#include <init.capnp.proxy-types.h>
9+#include <init.capnp.proxy.h> // NOLINT(misc-include-cleaner)

ryanofsky commented at 5:33 pm on February 3, 2025:

In commit “clang-tidy: Fix misc-include-cleaner check” (90cb5a90174130290fc9390c49bdc7a041701db3)

Is the include-cleaner check not compatible with IWYU? When I ran IWYU it seemed to correctly add <init.capnp.proxy.h> include. Do we know if it is expected that clang-tidy would complain about this?

Same applies to other nolint below.

hebasto commented at 12:02 pm on February 4, 2025:

Is the include-cleaner check not compatible with IWYU?

They seem incompatible.

When I ran IWYU it seemed to correctly add <init.capnp.proxy.h> include.

The include-what-you-use tool built from the clang_19 branch still insist on removing #include <init.capnp.proxy.h>.

It seem both linters fail to parse a convoluted template code properly.

Additionally, IWYU suggests:

0#include <kj/async.h>     // for evalLater
1#include <kj/common.h>    // for fwd, mv, ctor, implicitCast, instance
2#include <kj/memory.h>    // for heap

for this line:https://github.com/chaincodelabs/libmultiprocess/blob/477405eda34d923bd2ba6b3abc4c4d31db84c3ea/example/calculator.cpp#L48

in test/mp/test/foo.h:24 in 19d7537cb5

20@@ -21,7 +21,7 @@ struct FooStruct
21     std::vector<bool> vbool;
22 };
23 
24-enum class FooEnum : int { ONE = 1, TWO = 2, };
25+enum class FooEnum : int { ONE = 1, TWO = 2, }; // NOLINT(performance-enum-size)

ryanofsky commented at 5:35 pm on February 3, 2025:

In commit “clang-tidy: Suppress performance-enum-size check warning” (19d7537cb52eef623439e1dd6b562524021ae1a4)

This seems like another thing that would be nice to fix instead of nolint. These things can be done as followups since it is easy to search for nolint.

vasild commented at 3:31 pm on February 4, 2025:

This works:

0-enum class FooEnum : int { ONE = 1, TWO = 2, }; // NOLINT(performance-enum-size)
1+enum class FooEnum : uint8_t { ONE = 1, TWO = 2, };

ryanofsky commented at 4:15 am on February 5, 2025:

re: https://github.com/chaincodelabs/libmultiprocess/pull/144#discussion_r1941401788

This is done in #152

ryanofsky approved

ryanofsky commented at 5:44 pm on February 3, 2025: collaborator

Code review ACK 19d7537cb52eef623439e1dd6b562524021ae1a4. Looks great, thanks for fixing all these errors and packaging up the changes so nicely. I started fixing some of these myself, but did not get very far and you did a nicer job.

There are a number of things that could be followed up on here, mentioned in comments. But it should be easy to remaining issues by searching for nolint or modifying .clang-tidy file to include more warnings.

fanquake commented at 5:47 pm on February 3, 2025: member

It’d be good if the commit messages explained what they are doing, and why. The choice to suppress or fix seems completely arbitrary here, and where suppress is done, there’s no explanation why.

ryanofsky commented at 5:52 pm on February 3, 2025: collaborator

The choice to suppress or fix seems completely arbitrary here, and where suppress is done, there’s no explanation why.

I just assumed choice to suppress or fix was to make libmultiprocess tidy option match bitcoin tidy options? But maybe this is not the case

hebasto commented at 12:06 pm on February 4, 2025: member

@ryanofsky

Looks great, thanks for fixing all these errors and packaging up the changes so nicely.

Thank you!

I started fixing some of these myself, but did not get very far and you did a nicer job.

There are a number of things that could be followed up on here, mentioned in comments.

Do you mind stepping in and continuing?

ryanofsky commented at 1:40 pm on February 4, 2025: collaborator

Do you mind stepping in and continuing?

Sure, I’d be happy to merge this PR as-is. It just has a conflict that nees to be resolved. IMO it just be good to drop the first commit a6920380144a96bf0b127c0ae2d49c86deb826b1 which is causing the conflict, and since I think it would be better to keep checking generated files.

Just let me know what you’d prefer: (1) You drop commit or fix conflict here and I merge this PR? (2) I update your branch (if I have permission) and merge it? (3) We close and open a new PR?

hebasto commented at 1:55 pm on February 4, 2025: member

(3) We close and open a new PR?

I’ll be happy to review and test it.

hebasto closed this on Feb 4, 2025

in src/mp/proxy.cpp:226 in 19d7537cb5

223 {
224     if (std::this_thread::get_id() == m_thread_id) {
225-        return fn();
226+        fn();
227+        return;
228     }

vasild commented at 3:23 pm on February 4, 2025:

f159c75 clang-tidy: Fix readability-avoid-return-with-void-value check

There is similar, (undetected?) code in include/mp/proxy-io.h in EventLoop::sync() which warrants:

0  void sync(Callable&& callable)
1  {
2-      return post(std::ref(callable));
3+      post(std::ref(callable));
4  }

ryanofsky commented at 4:14 am on February 5, 2025:

re: https://github.com/chaincodelabs/libmultiprocess/pull/144#discussion_r1941387017

This is done in #152

vasild commented at 3:53 pm on February 4, 2025: contributor

There is one warning that is not fixed by this PR:

0libmultiprocess/test/mp/test/foo.h:71:9: warning: method 'callbackSaved' can be made const [readability-make-member-function-const]
1   71 |     int callbackSaved(int arg) { return m_callback->call(arg); }
2      |         ^
3      |                                const

ryanofsky commented at 4:47 pm on February 4, 2025: collaborator

Thanks vasild! If you have any interest in opening a PR, would be happy to review it. Also fine if not, all the information you’ve gathered here will already be helpful to follow up.

ryanofsky commented at 3:27 am on February 5, 2025: collaborator

re: https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2628639378

Still planning on cleaning up the warnings anyway since I already fixed a lot of them locally.

I’ve just read it. My apologies for overstepping.

Just saw this message. Definitely appreciate this and your other PRs. They have all be really helpful and not overstepping in any way.

ryanofsky referenced this in commit 9558ceb0d4 on Feb 5, 2025

hebasto deleted the branch on Feb 5, 2025

ryanofsky referenced this in commit 02f58e0b24 on Feb 10, 2025

ryanofsky referenced this in commit 8437c77046 on Apr 24, 2025

ryanofsky referenced this in commit 4b97111ada on Apr 24, 2025

ryanofsky referenced this in commit 8cae952520 on Apr 24, 2025

ryanofsky referenced this in commit 25baba9f53 on May 15, 2025

ryanofsky referenced this in commit 4b0296395a on May 15, 2025

ryanofsky referenced this in commit 84b3fb6d10 on May 30, 2025

ryanofsky referenced this in commit 9dbcb2e285 on May 30, 2025

ryanofsky referenced this in commit 7aad50529b on Jun 5, 2025

ryanofsky referenced this in commit 4f83172087 on Jun 13, 2025

ryanofsky referenced this in commit df1be80ad3 on Jun 13, 2025

ryanofsky referenced this in commit bb2cf204d8 on Jun 13, 2025

ryanofsky referenced this in commit a8e9cffc6b on Jun 13, 2025

ryanofsky referenced this in commit 52256e730f on Jun 16, 2025

ryanofsky referenced this in commit 258a617c1e on Jun 19, 2025

Address clang-tidy-19 warnings #144

Address `clang-tidy-19` warnings #144