Introduce `clang-tidy` and optimize code #83

pull hebasto wants to merge 10 commits into bitcoin-core:master from hebasto:230126-tidy changing 9 files +60 −11
  1. hebasto commented at 9:43 PM on January 26, 2023: member

    This PR:

    • introduces the -DLibmultiprocess_ENABLE_CLANG_TIDY configuration option, which allows to "run clang-tidy with the compiler"
    • fixes some clang-tidy's warnings, which are easy-to-review and useful on their own
  2. Sjors commented at 10:44 AM on February 11, 2023: member

    I got one warning during make, same as on master, building on Ubuntu 22.10 with clang 15.0.6:

    [ 75%] Built target multiprocess
[ 87%] Building CXX object CMakeFiles/mpgen.dir/src/mp/gen.cpp.o
/home/sjors/dev/libmultiprocess/src/mp/gen.cpp: In function ‘void Generate(kj::StringPtr, kj::StringPtr, kj::StringPtr, kj::ArrayPtr<const kj::StringPtr>)’:
/home/sjors/dev/libmultiprocess/src/mp/gen.cpp:179:44: warning: ‘capnp::ParsedSchema capnp::SchemaParser::parseDiskFile(kj::StringPtr, kj::StringPtr, kj::ArrayPtr<const kj::StringPtr>) const’ is deprecated [-Wdeprecated-declarations]
  179 |     auto file_schema = parser.parseDiskFile(src_file, src_file, import_paths);
      |                        ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/sjors/dev/libmultiprocess/src/mp/gen.cpp:9:
/usr/include/capnp/schema-parser.h:101:16: note: declared here
  101 |   ParsedSchema parseDiskFile(kj::StringPtr displayName, kj::StringPtr diskPath,
      |                ^~~~~~~~~~~~~
[100%] Linking CXX executable mpgen
[100%] Built target mpgen

    make check fails entirely though, while on master it works fine.

    [ 73%] Building CXX object test/CMakeFiles/mptest.dir/mp/test/foo.capnp.proxy-server.c++.o
In file included from /home/sjors/dev/libmultiprocess/include/mp/proxy.h:8,
                 from /home/sjors/dev/libmultiprocess/build/test/mp/test/foo.capnp.proxy.h:8,
                 from /home/sjors/dev/libmultiprocess/build/test/mp/test/foo.capnp.proxy-types.h:6,
                 from /home/sjors/dev/libmultiprocess/build/test/mp/test/foo.capnp.proxy-server.c++:3:
/home/sjors/dev/libmultiprocess/include/mp/util.h: In instantiation of ‘mp::AsyncCallable<typename std::remove_reference<_T
…
/home/sjors/dev/libmultiprocess/build/test/mp/test/foo.capnp.proxy-server.c++:23:24:   required from here
/home/sjors/dev/libmultiprocess/include/mp/util.h:349:24: error: no matching function for call to ‘forward(kj::CaptureByMove<kj::CaptureByMove<mp::PassField<Accessor<foo_fields::Context, 17>,
…
ooInterface::CallbackResults> > > >&)’
  349 |     return std::forward(callable);
      |            ~~~~~~~~~~~~^~~~~~~~~~
In file included from /usr/include/c++/12/bits/stl_pair.h:61,
                 from /usr/include/c++/12/bits/stl_algobase.h:64,
                 from /usr/include/c++/12/bits/stl_tree.h:63,
                 from /usr/include/c++/12/map:60,
                 from /home/sjors/dev/libmultiprocess/test/mp/test/foo.h:8,
                 from /home/sjors/dev/libmultiprocess/build/test/mp/test/foo.capnp.proxy.h:7:
/usr/include/c++/12/bits/move.h:77:5: note: candidate: ‘template<class _Tp> constexpr _Tp&& std::forward(typename remove_reference<_Tp>::type&)’
   77 |     forward(typename std::remove_reference<_Tp>::type& __t) noexcept

    full log

  3. hebasto force-pushed on Feb 12, 2023
  4. hebasto commented at 2:26 PM on February 12, 2023: member

    @Sjors

    Thank you for your comment.

    make check fails entirely though, while on master it works fine.

    The PR has been reworked now.

  5. Sjors commented at 2:35 PM on February 12, 2023: member

    make check works with ceb3a4b8a0d5d04d7a8573a3dca4f71b4a8b04da

  6. ryanofsky commented at 9:00 PM on February 13, 2023: collaborator

    re: https://github.com/chaincodelabs/libmultiprocess/pull/83#issuecomment-1426695225

    Note: warning about deprecated parseDiskFile call is reported as a standalone issue in #39

  7. in CMakeLists.txt:11 in f3b2bfcbe7 outdated 
       7 | @@ -8,6 +8,15 @@ project("Libmultiprocess" CXX)
   8 |  set(CMAKE_CXX_STANDARD 17)
   9 |  set(CMAKE_CXX_STANDARD_REQUIRED YES)
  10 |  
  11 | +option(MULTIPROCESS_RUN_CLANG_TIDY "Run clang-tidy with the compiler." OFF)
    ryanofsky commented at 8:38 PM on February 14, 2023:

    In commit "Add option to run clang-tidy with compiler" (f3b2bfcbe7ecca087d75e96df1496687f8ff4c0a)

    Would it make sense to call the option ENABLE_CLANG_TIDY instead of MULTIPROCESS_RUN_CLANG_TIDY? I don't see a reason to prefix option names with the project name, and the other options don't do this

    hebasto commented at 10:00 PM on February 14, 2023:

    The goal of prefixing is to minimize risk of option name clash when this library is a subproject of another project. For example, https://github.com/bitcoin-core/secp256k1/pull/1113.

    ryanofsky commented at 10:25 PM on February 14, 2023:

    Ok, I didn't think about this being used as a subdirectory inside another project. Could we then prefix project options with the project name like Libmultiprocess_ENABLE_CLANG_TIDY. I do also prefer ENABLE to RUN for the cmake option name, because at the CMakeLists.txt level, this isn't adding more build steps, just setting another option.

    hebasto commented at 8:42 AM on February 15, 2023:

    Thanks! Updated.

  8. ryanofsky commented at 8:41 PM on February 14, 2023: collaborator

    Confirmed ceb3a4b8a0d5d04d7a8573a3dca4f71b4a8b04da is working and runs clang but I didn't review the individual commits yet.

    I also noticed one new warning is shown now (using clang-tidy 14.0.6)

    include/mp/proxy-types.h:162:85: warning: the parameter 'perhaps' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
                            .then([&server, invoke, req](kj::Maybe<Thread::Server&> perhaps) {
                                                                                    ^
                                                         const                     &
  9. in test/mp/test/foo.h:51 in 0498ed86bc outdated 
      47 | @@ -48,7 +48,7 @@ class FooImplementation
  48 |      void initThreadMap() {}
  49 |      int callback(FooCallback& callback, int arg) { return callback.call(arg); }
  50 |      int callbackUnique(std::unique_ptr<FooCallback> callback, int arg) { return callback->call(arg); }
  51 | -    int callbackShared(std::shared_ptr<FooCallback> callback, int arg) { return callback->call(arg); }
  52 | +    int callbackShared(std::shared_ptr<FooCallback> callback, int arg) { return callback->call(arg); } // NOLINT(performance-unnecessary-value-param)
    ryanofsky commented at 9:05 PM on February 14, 2023:

    In commit "clang-tidy: Fix performance-unnecessary-value-param check" (0498ed86bc6d1a6bdbabf98f2e39468dc5c90823)

    Note: I think it does make sense to suppress this lint warning than try to avoid it. Could potentially avoid it by changing this argument to const shared_ptr but there's currently no IPC serialization code to handle const shared_ptr only shared_ptr, so it does't compile. Also I don't think it would make sense to add const shared_ptr serialization code because most cases where you would pass const shared<ptr> it probably makes more sense to pass a plain const reference.

  10. ryanofsky commented at 9:08 PM on February 14, 2023: collaborator

    re: https://github.com/chaincodelabs/libmultiprocess/pull/83#pullrequestreview-1298380154

    I also noticed one new warning is shown now (using clang-tidy 14.0.6)

    Following change fixes the warning (EDIT: merged this change already in https://github.com/chaincodelabs/libmultiprocess/pull/84)

    --- a/include/mp/proxy-types.h
+++ b/include/mp/proxy-types.h
@@ -159,7 +159,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
 
     auto thread_client = context_arg.getThread();
     return JoinPromises(server.m_context.connection->m_threads.getLocalServer(thread_client)
-                            .then([&server, invoke, req](kj::Maybe<Thread::Server&> perhaps) {
+                            .then([&server, invoke, req](const kj::Maybe<Thread::Server&>& perhaps) {
                                 KJ_IF_MAYBE(thread_server, perhaps)
                                 {
                                     const auto& thread = static_cast<ProxyServer<Thread>&>(*thread_server);
  11. ryanofsky referenced this in commit 8ef94d2e86 on Feb 14, 2023
  12. ryanofsky referenced this in commit 74e25d2cd4 on Feb 14, 2023
  13. ryanofsky commented at 9:29 PM on February 14, 2023: collaborator

    Code review ACK ceb3a4b8a0d5d04d7a8573a3dca4f71b4a8b04da, but I think I'd like to rename the cmake option as described https://github.com/chaincodelabs/libmultiprocess/pull/83#discussion_r1106345466. I noticed some other clang-tidy errors still occurred for me with clang-tidy 14.0.6 after this PR, so I separately merged fixes for these in https://github.com/chaincodelabs/libmultiprocess/pull/84

  14. Add option to run `clang-tidy` with compiler 5e787bfbd0
  15. clang-tidy: Suppress `bugprone-suspicious-semicolon` check warning
    See https://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-semicolon.html
    8dd83bbd91
  16. clang-tidy: Fix `modernize-return-braced-init-list` check
    See https://clang.llvm.org/extra/clang-tidy/checks/modernize/return-braced-init-list.html
    1a33c35a3b
  17. clang-tidy: Fix `modernize-use-emplace` check
    See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html
    18b52c1663
  18. clang-tidy: Fix `modernize-use-equals-default` check
    See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html
    9f86c9a0a8
  19. clang-tidy: Fix `modernize-use-nullptr` check
    See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-nullptr.html
    ae416f903f
  20. clang-tidy: Fix `performance-faster-string-find` check
    See https://clang.llvm.org/extra/clang-tidy/checks/performance/faster-string-find.html
    a435b24e64
  21. clang-tidy: Fix `performance-inefficient-vector-operation` check
    See https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html
    463bead1f3
  22. clang-tidy: Fix `performance-unnecessary-value-param` check
    See https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-value-param.html
    037fec47c8
  23. clang-tidy: Fix `readability-make-member-function-const` check
    See https://clang.llvm.org/extra/clang-tidy/checks/readability/make-member-function-const.html
    594466ae97
  24. hebasto force-pushed on Feb 15, 2023
  25. hebasto commented at 8:41 AM on February 15, 2023: member

    Updated 78bfa2a2812fe56b172ca4f16e7c332d0e03f103 -> d795e969dd7c913c937a4f97d78115072feb6cb4 (pr83.02 -> pr83.03):

    • rebased on top of the #84
    • addressed @ryanofsky's comment
    • fixed HeaderFilterRegex for the example directory
  26. ryanofsky approved
  27. ryanofsky commented at 3:23 PM on February 15, 2023: collaborator

    Code review ACK 594466ae975a8370bfed8f6de05bbcf92289dd8f. Changes since last review were changing option name, and updating HeaderFilterRegex, and rebasing

  28. ryanofsky merged this on Feb 15, 2023
  29. ryanofsky closed this on Feb 15, 2023
  30. ryanofsky commented at 3:26 PM on February 15, 2023: collaborator

    Merged now. Thank you for these nice changes and features!

  31. hebasto deleted the branch on Feb 15, 2023
  32. bitcoin-core locked this on Jun 25, 2025
Contributors
hebastoSjorsryanofsky
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/libmultiprocess. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-20 19:30 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me