Introduce `clang-tidy` and optimize code by hebasto · Pull Request #83 · bitcoin-core/libmultiprocess

hebasto · 2023-01-26T21:43:05Z

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

Sjors · 2023-02-11T10:44:41Z

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

hebasto · 2023-02-12T14:26:29Z

@Sjors

Thank you for your comment.

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

The PR has been reworked now.

Sjors · 2023-02-12T14:35:34Z

make check works with ceb3a4b

ryanofsky · 2023-02-13T21:00:34Z

re: #83 (comment)

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

ryanofsky

Confirmed ceb3a4b 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                     &

ryanofsky · 2023-02-14T20:38:20Z

CMakeLists.txt

 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED YES)

+option(MULTIPROCESS_RUN_CLANG_TIDY "Run clang-tidy with the compiler." OFF)


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

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

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

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.

Thanks! Updated.

ryanofsky · 2023-02-14T21:05:59Z

test/mp/test/foo.h

    int callback(FooCallback& callback, int arg) { return callback.call(arg); }
    int callbackUnique(std::unique_ptr<FooCallback> callback, int arg) { return callback->call(arg); }
-    int callbackShared(std::shared_ptr<FooCallback> callback, int arg) { return callback->call(arg); }
+    int callbackShared(std::shared_ptr<FooCallback> callback, int arg) { return callback->call(arg); } // NOLINT(performance-unnecessary-value-param)


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

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.

ryanofsky · 2023-02-14T21:08:05Z

re: #83 (review)

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 #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);

Preemptively avoid clang-tidy errors before clang-tidy is introduced in bitcoin-core#83. Other clang errors are already fixed in that PR, these aren't (maybe due to running different clang-tidy versions). The errors look like: 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 & example/printer.cpp:27:39: warning: the parameter 'message' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] void LogPrint(bool raise, std::string message) ^ example/printer.cpp:29:41: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] if (raise) throw std::runtime_error(std::move(message)); ^~~~~~~~~~ ~

8ef94d2 Avoid passing some function arguments by value (Ryan Ofsky) Pull request description: Preemptively avoid clang-tidy errors before clang-tidy is introduced in #83. Other clang errors are already fixed in that PR, but these aren't maybe due to running different clang-tidy versions. The errors look like: ```c++ 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 & example/printer.cpp:27:39: warning: the parameter 'message' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] void LogPrint(bool raise, std::string message) ^ example/printer.cpp:29:41: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] if (raise) throw std::runtime_error(std::move(message)); ^~~~~~~~~~ ~ ``` Top commit has no ACKs. Tree-SHA512: 3aac30ac42ad0cde07f67efcd330d11ac450e8a1a8c68ffa3129a1ef519345473c9c6fe6a0746c07bb18d1874e30c328404e2bcd65582e8a770614193bf8306a

ryanofsky · 2023-02-14T21:29:59Z

Code review ACK ceb3a4b, but I think I'd like to rename the cmake option as described #83 (comment). 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 #84

See https://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-semicolon.html

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/return-braced-init-list.html

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-nullptr.html

See https://clang.llvm.org/extra/clang-tidy/checks/performance/faster-string-find.html

See https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html

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

See https://clang.llvm.org/extra/clang-tidy/checks/readability/make-member-function-const.html

hebasto · 2023-02-15T08:41:59Z

Updated 78bfa2a -> d795e96 (pr83.02 -> pr83.03):

rebased on top of the Avoid passing some function arguments by value #84
addressed @ryanofsky's comment
fixed HeaderFilterRegex for the example directory

ryanofsky

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

ryanofsky · 2023-02-15T15:26:06Z

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

hebasto force-pushed the 230126-tidy branch from 0ac51eb to ceb3a4b Compare February 12, 2023 14:24

ryanofsky reviewed Feb 14, 2023

View reviewed changes

ryanofsky mentioned this pull request Feb 14, 2023

Avoid passing some function arguments by value #84

Merged

hebasto added 10 commits February 15, 2023 08:37

Add option to run clang-tidy with compiler

5e787bf

clang-tidy: Suppress bugprone-suspicious-semicolon check warning

8dd83bb

See https://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-semicolon.html

clang-tidy: Fix modernize-return-braced-init-list check

1a33c35

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/return-braced-init-list.html

clang-tidy: Fix modernize-use-emplace check

18b52c1

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html

clang-tidy: Fix modernize-use-equals-default check

9f86c9a

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html

clang-tidy: Fix modernize-use-nullptr check

ae416f9

See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-nullptr.html

clang-tidy: Fix performance-faster-string-find check

a435b24

See https://clang.llvm.org/extra/clang-tidy/checks/performance/faster-string-find.html

clang-tidy: Fix performance-inefficient-vector-operation check

463bead

See https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html

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

037fec4

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

clang-tidy: Fix readability-make-member-function-const check

594466a

See https://clang.llvm.org/extra/clang-tidy/checks/readability/make-member-function-const.html

hebasto force-pushed the 230126-tidy branch from ceb3a4b to 594466a Compare February 15, 2023 08:37

ryanofsky approved these changes Feb 15, 2023

View reviewed changes

ryanofsky merged commit fc28a48 into bitcoin-core:master Feb 15, 2023

hebasto deleted the 230126-tidy branch February 15, 2023 15:31

ryanofsky mentioned this pull request Nov 17, 2023

depends: Bump to capnproto-c++-1.0.1 bitcoin/bitcoin#28735

Merged

hebasto mentioned this pull request Jan 31, 2025

Address clang-tidy-19 warnings #144

Closed

bitcoin-core locked and limited conversation to collaborators Jun 25, 2025

Conversation

hebasto commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 11, 2023

Uh oh!

hebasto commented Feb 12, 2023

Uh oh!

Sjors commented Feb 12, 2023

Uh oh!

ryanofsky commented Feb 13, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 14, 2023 • 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 Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 15, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Feb 14, 2023

Uh oh!

hebasto commented Feb 15, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Feb 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hebasto commented Jan 26, 2023 •

edited

Loading

hebasto Feb 14, 2023 •

edited

Loading

ryanofsky commented Feb 14, 2023 •

edited

Loading