Merge #152: refactor: Fix compiler and clang-tidy warnings

0b8b7f9 refactor: Fix `-Wsign-compare` compiler warning (Hennadii Stepanov)
a02c079 clang-tidy: Suppress `performance-enum-size` check warning (Hennadii Stepanov)
593807a clang-tidy: Fix `readability-container-size-empty` check (Hennadii Stepanov)
68c1c6c clang-tidy: Fix `readability-avoid-return-with-void-value` check (Hennadii Stepanov)
4abaa98 clang-tidy: Fix `readability-avoid-nested-conditional-operator` check (Hennadii Stepanov)
01ef094 clang-tidy: Fix `performance-unnecessary-value-param` check (Hennadii Stepanov)
c665a43 clang-tidy: Fix `misc-use-internal-linkage` check (Hennadii Stepanov)
15c77e9 clang-tidy: Fix `misc-include-cleaner` check (Hennadii Stepanov)
848c902 clang-tidy: Fix `misc-const-correctness` check (Hennadii Stepanov)
4a2508c clang-tidy: Fix `modernize-type-traits` check (Hennadii Stepanov)
8170d3d clang-tidy: Suppress `bugprone-empty-catch` check warning (Hennadii Stepanov)
c068596 clang-tidy: Fix `bugprone-crtp-constructor-accessibility` check (Hennadii Stepanov)
00036c0 clang-tidy: Disable `performance-avoid-endl` check (Hennadii Stepanov)
359a615 clang-tidy: Disable `misc-use-anonymous-namespace` check (Hennadii Stepanov)

Pull request description:

  Most of these changes are not my own. This is combination of commits from #144 and #151 which fix a variety of compiler warnings.

  Several of the fixes are just suppressions, so followups can be done to improve code and fix problems more completely. More details can be found in #144.

Top commit has no ACKs.

Tree-SHA512: 581720d357dce36553f91fb4bf92c4ade2c228ce199b342c28637f2615b22aa6e5908c83a698320438e5d53309581ec64410f469cbb782843970caee3c21d62e

Author: ryanofsky

.clang-tidy

@@ -11,6 +11,7 @@ misc-*,
 -misc-no-recursion,
 -misc-unconventional-assign-operator,
 -misc-unused-parameters,
+-misc-use-anonymous-namespace,
 modernize-*,
 -modernize-avoid-c-arrays,
 -modernize-concat-nested-namespaces,
@@ -19,6 +20,7 @@ modernize-*,
 -modernize-use-trailing-return-type,
 -modernize-use-using,
 performance-*,
+-performance-avoid-endl,
 -performance-noexcept-move-constructor,
 readability-*,
 -readability-braces-around-statements,

example/calculator.cpp

@@ -5,13 +5,15 @@
 #include <calculator.h>
 #include <fstream>
 #include <init.capnp.h>
-#include <init.capnp.proxy-types.h>
+#include <init.capnp.proxy.h> // NOLINT(misc-include-cleaner)
 #include <init.h>
 #include <iostream>
 #include <memory>
 #include <mp/proxy-io.h>
 #include <printer.h>
 #include <stdexcept>
+#include <string>
+#include <utility>
 
 class CalculatorImpl : public Calculator
 {
@@ -30,7 +32,7 @@ class InitImpl : public Init
     }
 };
 
-void LogPrint(bool raise, const std::string& message)
+static void LogPrint(bool raise, const std::string& message)
 {
     if (raise) throw std::runtime_error(message);
     std::ofstream("debug.log", std::ios_base::app) << message << std::endl;
@@ -43,7 +45,7 @@ int main(int argc, char** argv)
         return 1;
     }
     mp::EventLoop loop("mpcalculator", LogPrint);
-    int fd = std::stoi(argv[1]);
+    const int fd = std::stoi(argv[1]);
     std::unique_ptr<Init> init = std::make_unique<InitImpl>();
     mp::ServeStream<InitInterface>(loop, fd, *init);
     loop.loop();

example/example.cpp

@@ -4,17 +4,24 @@
 
 #include <filesystem>
 #include <fstream>
+#include <future>
 #include <init.capnp.h>
-#include <init.capnp.proxy-types.h>
+#include <init.capnp.proxy.h>
 #include <iostream>
 #include <mp/proxy-io.h>
+#include <mp/util.h>
+#include <stdexcept>
+#include <string>
+#include <thread>
+#include <tuple>
+#include <vector>
 
 namespace fs = std::filesystem;
 
-auto Spawn(mp::EventLoop& loop, const std::string& process_argv0, const std::string& new_exe_name)
+static auto Spawn(mp::EventLoop& loop, const std::string& process_argv0, const std::string& new_exe_name)
 {
     int pid;
-    int fd = mp::SpawnProcess(pid, [&](int fd) -> std::vector<std::string> {
+    const int fd = mp::SpawnProcess(pid, [&](int fd) -> std::vector<std::string> {
         fs::path path = process_argv0;
         path.remove_filename();
         path.append(new_exe_name);
@@ -23,7 +30,7 @@ auto Spawn(mp::EventLoop& loop, const std::string& process_argv0, const std::str
     return std::make_tuple(mp::ConnectStream<InitInterface>(loop, fd), pid);
 }
 
-void LogPrint(bool raise, const std::string& message)
+static void LogPrint(bool raise, const std::string& message)
 {
     if (raise) throw std::runtime_error(message);
     std::ofstream("debug.log", std::ios_base::app) << message << std::endl;

example/printer.cpp

@@ -4,13 +4,14 @@
 
 #include <fstream>
 #include <init.capnp.h>
-#include <init.capnp.proxy-types.h>
+#include <init.capnp.proxy.h> // NOLINT(misc-include-cleaner)
 #include <init.h>
 #include <iostream>
 #include <memory>
 #include <mp/proxy-io.h>
 #include <printer.h>
 #include <stdexcept>
+#include <string>
 
 class PrinterImpl : public Printer
 {
@@ -24,7 +25,7 @@ class InitImpl : public Init
     std::unique_ptr<Printer> makePrinter() override { return std::make_unique<PrinterImpl>(); }
 };
 
-void LogPrint(bool raise, const std::string& message)
+static void LogPrint(bool raise, const std::string& message)
 {
     if (raise) throw std::runtime_error(message);
     std::ofstream("debug.log", std::ios_base::app) << message << std::endl;
@@ -37,7 +38,7 @@ int main(int argc, char** argv)
         return 1;
     }
     mp::EventLoop loop("mpprinter", LogPrint);
-    int fd = std::stoi(argv[1]);
+    const int fd = std::stoi(argv[1]);
     std::unique_ptr<Init> init = std::make_unique<InitImpl>();
     mp::ServeStream<InitInterface>(loop, fd, *init);
     loop.loop();

include/mp/proxy-io.h

@@ -63,7 +63,7 @@ struct ProxyClient<Thread> : public ProxyClientBase<Thread, ::capnp::Void>
     ProxyClient(const ProxyClient&) = delete;
     ~ProxyClient();
 
-    void setCleanup(std::function<void()> fn);
+    void setCleanup(const std::function<void()>& fn);
 
     //! Cleanup function to run when the connection is closed. If the Connection
     //! gets destroyed before this ProxyClient<Thread> object, this cleanup
@@ -152,7 +152,7 @@ class EventLoop
     template <typename Callable>
     void sync(Callable&& callable)
     {
-        return post(std::ref(callable));
+        post(std::ref(callable));
     }
 
     //! Start asynchronous worker thread if necessary. This is only done if
@@ -247,7 +247,7 @@ struct Waiter
     template <typename Fn>
     void post(Fn&& fn)
     {
-        std::unique_lock<std::mutex> lock(m_mutex);
+        const std::unique_lock<std::mutex> lock(m_mutex);
         assert(!m_fn);
         m_fn = std::move(fn);
         m_cv.notify_all();
@@ -269,7 +269,7 @@ struct Waiter
                 fn();
                 lock.lock();
             }
-            bool done = pred();
+            const bool done = pred();
             return done;
         });
     }
@@ -488,7 +488,7 @@ ProxyServerBase<Interface, Impl>::~ProxyServerBase()
             CleanupRun(fns);
         });
     }
-    assert(m_context.cleanup_fns.size() == 0);
+    assert(m_context.cleanup_fns.empty());
     std::unique_lock<std::mutex> lock(m_context.connection->m_loop.m_mutex);
     m_context.connection->m_loop.removeClient(lock);
 }
@@ -523,7 +523,7 @@ using ConnThread = ConnThreads::iterator;
 // Retrieve ProxyClient<Thread> object associated with this connection from a
 // map, or create a new one and insert it into the map. Return map iterator and
 // inserted bool.
-std::tuple<ConnThread, bool> SetThread(ConnThreads& threads, std::mutex& mutex, Connection* connection, std::function<Thread::Client()> make_thread);
+std::tuple<ConnThread, bool> SetThread(ConnThreads& threads, std::mutex& mutex, Connection* connection, const std::function<Thread::Client()>& make_thread);
 
 struct ThreadContext
 {

include/mp/proxy-types.h

@@ -39,17 +39,17 @@ struct StructField
 
     // clang-format off
     template<typename A = Accessor> auto get() const -> decltype(A::get(this->m_struct)) { return A::get(this->m_struct); }
-    template<typename A = Accessor> auto has() const -> typename std::enable_if<A::optional, bool>::type { return A::getHas(m_struct); }
-    template<typename A = Accessor> auto has() const -> typename std::enable_if<!A::optional && A::boxed, bool>::type { return A::has(m_struct); }
-    template<typename A = Accessor> auto has() const -> typename std::enable_if<!A::optional && !A::boxed, bool>::type { return true; }
-    template<typename A = Accessor> auto want() const -> typename std::enable_if<A::requested, bool>::type { return A::getWant(m_struct); }
-    template<typename A = Accessor> auto want() const -> typename std::enable_if<!A::requested, bool>::type { return true; }
+    template<typename A = Accessor> auto has() const -> std::enable_if_t<A::optional, bool> { return A::getHas(m_struct); }
+    template<typename A = Accessor> auto has() const -> std::enable_if_t<!A::optional && A::boxed, bool> { return A::has(m_struct); }
+    template<typename A = Accessor> auto has() const -> std::enable_if_t<!A::optional && !A::boxed, bool> { return true; }
+    template<typename A = Accessor> auto want() const -> std::enable_if_t<A::requested, bool> { return A::getWant(m_struct); }
+    template<typename A = Accessor> auto want() const -> std::enable_if_t<!A::requested, bool> { return true; }
     template<typename A = Accessor, typename... Args> decltype(auto) set(Args&&... args) const { return A::set(this->m_struct, std::forward<Args>(args)...); }
     template<typename A = Accessor, typename... Args> decltype(auto) init(Args&&... args) const { return A::init(this->m_struct, std::forward<Args>(args)...); }
-    template<typename A = Accessor> auto setHas() const -> typename std::enable_if<A::optional>::type { return A::setHas(m_struct); }
-    template<typename A = Accessor> auto setHas() const -> typename std::enable_if<!A::optional>::type { }
-    template<typename A = Accessor> auto setWant() const -> typename std::enable_if<A::requested>::type { return A::setWant(m_struct); }
-    template<typename A = Accessor> auto setWant() const -> typename std::enable_if<!A::requested>::type { }
+    template<typename A = Accessor> auto setHas() const -> std::enable_if_t<A::optional> { return A::setHas(m_struct); }
+    template<typename A = Accessor> auto setHas() const -> std::enable_if_t<!A::optional> { }
+    template<typename A = Accessor> auto setWant() const -> std::enable_if_t<A::requested> { return A::setWant(m_struct); }
+    template<typename A = Accessor> auto setWant() const -> std::enable_if_t<!A::requested> { }
     // clang-format on
 };
 
@@ -314,6 +314,9 @@ struct IterateFieldsHelper
     {
         static_cast<Derived*>(this)->handleField(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), ParamList());
     }
+private:
+    IterateFieldsHelper() = default;
+    friend Derived;
 };
 
 struct IterateFields : IterateFieldsHelper<IterateFields, 0>
@@ -372,14 +375,14 @@ struct ClientParam
         // position when unpacking tuple might be slower than pattern matching
         // approach in the stack overflow solution
         template <size_t I, typename... Args>
-        auto callBuild(Args&&... args) -> typename std::enable_if<(I < sizeof...(Types))>::type
+        auto callBuild(Args&&... args) -> std::enable_if_t<(I < sizeof...(Types))>
         {
             callBuild<I + 1>(std::forward<Args>(args)..., std::get<I>(m_client_param->m_values));
         }
 
         template <size_t I, typename Params, typename ParamList, typename... Values>
         auto callBuild(ClientInvokeContext& invoke_context, Params& params, ParamList, Values&&... values) ->
-            typename std::enable_if<(I == sizeof...(Types))>::type
+            std::enable_if_t<(I == sizeof...(Types))>
         {
             MaybeBuildField(std::integral_constant<bool, Accessor::in>(), ParamList(), invoke_context,
                 Make<StructField, Accessor>(params), std::forward<Values>(values)...);
@@ -400,14 +403,14 @@ struct ClientParam
         }
 
         template <int I, typename... Args>
-        auto callRead(Args&&... args) -> typename std::enable_if<(I < sizeof...(Types))>::type
+        auto callRead(Args&&... args) -> std::enable_if_t<(I < sizeof...(Types))>
         {
             callRead<I + 1>(std::forward<Args>(args)..., std::get<I>(m_client_param->m_values));
         }
 
         template <int I, typename Results, typename... Params, typename... Values>
         auto callRead(ClientInvokeContext& invoke_context, Results& results, TypeList<Params...>, Values&&... values)
-            -> typename std::enable_if<I == sizeof...(Types)>::type
+            -> std::enable_if_t<I == sizeof...(Types)>
         {
             MaybeReadField(std::integral_constant<bool, Accessor::out>(), TypeList<Decay<Params>...>(), invoke_context,
                 Make<StructField, Accessor>(results), ReadDestUpdate(values)...);
@@ -623,15 +626,15 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel
                 } catch (...) {
                     exception = std::current_exception();
                 }
-                std::unique_lock<std::mutex> lock(invoke_context.thread_context.waiter->m_mutex);
+                const std::unique_lock<std::mutex> lock(invoke_context.thread_context.waiter->m_mutex);
                 done = true;
                 invoke_context.thread_context.waiter->m_cv.notify_all();
             },
             [&](const ::kj::Exception& e) {
                 kj_exception = kj::str("kj::Exception: ", e).cStr();
                 proxy_client.m_context.connection->m_loop.logPlain()
                     << "{" << invoke_context.thread_context.thread_name << "} IPC client exception " << kj_exception;
-                std::unique_lock<std::mutex> lock(invoke_context.thread_context.waiter->m_mutex);
+                const std::unique_lock<std::mutex> lock(invoke_context.thread_context.waiter->m_mutex);
                 done = true;
                 invoke_context.thread_context.waiter->m_cv.notify_all();
             }));
@@ -648,7 +651,7 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel
 //! duplication and branching in generic code that forwards calls to functions.
 template <typename Fn, typename Ret>
 auto ReplaceVoid(Fn&& fn, Ret&& ret) ->
-    typename std::enable_if<std::is_same<void, decltype(fn())>::value, decltype(ret())>::type
+    std::enable_if_t<std::is_same_v<void, decltype(fn())>, decltype(ret())>
 {
     fn();
     return ret();
@@ -657,7 +660,7 @@ auto ReplaceVoid(Fn&& fn, Ret&& ret) ->
 //! Overload of above for non-void `fn()` case.
 template <typename Fn, typename Ret>
 auto ReplaceVoid(Fn&& fn, Ret&& ret) ->
-    typename std::enable_if<!std::is_same<void, decltype(fn())>::value, decltype(fn())>::type
+    std::enable_if_t<!std::is_same_v<void, decltype(fn())>, decltype(fn())>
 {
     return fn();
 }

include/mp/proxy.h

@@ -198,7 +198,7 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)>
     template <size_t N>
     using Param = typename std::tuple_element<N, std::tuple<_Params...>>::type;
     using Fields =
-        typename std::conditional<std::is_same<void, Result>::value, Params, TypeList<_Params..., _Result>>::type;
+        std::conditional_t<std::is_same_v<void, Result>, Params, TypeList<_Params..., _Result>>;
 };
 
 //! Traits class for a proxy method, providing the same

include/mp/util.h

@@ -43,9 +43,9 @@ struct TypeList
 //! Example:
 //!   Make<std::pair>(5, true) // Constructs std::pair<int, bool>(5, true);
 template <template <typename...> class Class, typename... Types, typename... Args>
-Class<Types..., typename std::remove_reference<Args>::type...> Make(Args&&... args)
+Class<Types..., std::remove_reference_t<Args>...> Make(Args&&... args)
 {
-    return Class<Types..., typename std::remove_reference<Args>::type...>{std::forward<Args>(args)...};
+    return Class<Types..., std::remove_reference_t<Args>...>{std::forward<Args>(args)...};
 }
 
 //! Type helper splitting a TypeList into two halves at position index.
@@ -83,7 +83,7 @@ using RemoveCvRef = std::remove_cv_t<std::remove_reference_t<T>>;
 
 //! Type helper abbreviating std::decay.
 template <typename T>
-using Decay = typename std::decay<T>::type;
+using Decay = std::decay_t<T>;
 
 //! SFINAE helper, see using Require below.
 template <typename SfinaeExpr, typename Result_>
@@ -142,7 +142,7 @@ struct UnlockGuard
 template <typename Lock, typename Callback>
 void Unlock(Lock& lock, Callback&& callback)
 {
-    UnlockGuard<Lock> unlock(lock);
+    const UnlockGuard<Lock> unlock(lock);
     callback();
 }
 
@@ -157,7 +157,7 @@ struct DestructorCatcher
     {
     }
     ~DestructorCatcher() noexcept try {
-    } catch (const kj::Exception& e) {
+    } catch (const kj::Exception& e) { // NOLINT(bugprone-empty-catch)
     }
 };
 
@@ -181,7 +181,7 @@ struct AsyncCallable
 
 //! Construct AsyncCallable object.
 template <typename Callable>
-AsyncCallable<typename std::remove_reference<Callable>::type> MakeAsyncCallable(Callable&& callable)
+AsyncCallable<std::remove_reference_t<Callable>> MakeAsyncCallable(Callable&& callable)
 {
     return std::move(callable);
 }