Merge pull request #666 from zeromq/warnings

fix: fix compiler warnings, sign-conversion, clang-tidy issues
zeromq · Oct 23, 2024 · bde7474 · bde7474
2 parents 84b8866 + e908698
commit bde7474
Show file tree

Hide file tree

Showing 33 changed files with 823 additions and 566 deletions.
diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml
@@ -139,7 +139,8 @@ jobs:
       - name: Build Native Windows 32
         if: ${{ matrix.os == 'windows-2019' && matrix.node_arch == 'ia32' }}
         run:
-          node ./node_modules/@aminya/cmake-ts/build/main.js named-configs windows-x86
+          node ./node_modules/@aminya/cmake-ts/build/main.js named-configs
+          windows-x86
 
       - name: Use Node 20
         if: ${{ !matrix.docker }}

diff --git a/.mocharc.js b/.mocharc.js
@@ -8,6 +8,8 @@ const config = {
   "v8-expose-gc": true,
   exit: true,
   parallel: true,
+  timeout: 10000,
+  retries: 3,
 }
 
 module.exports = config
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -102,7 +102,12 @@ project_options(ENABLE_CACHE ENABLE_COMPILE_COMMANDS_SYMLINK)
 file(GLOB_RECURSE SOURCES "./src/*.cc")
 add_library(addon SHARED ${SOURCES})
 
-target_link_libraries(addon PRIVATE project_options) # project_warnings
+if(CMAKE_CXX_COMPILER_ID STREQUAL GNU
+   OR CMAKE_CXX_COMPILER_ID STREQUAL Clang
+   OR CMAKE_CXX_COMPILER_ID STREQUAL AppleClang)
+  target_compile_options(project_warnings INTERFACE "-Wno-shadow")
+endif()
+target_link_libraries(addon PRIVATE project_options project_warnings)
 
 if(ZMQ_DRAFT)
   target_compile_definitions(addon PRIVATE ZMQ_BUILD_DRAFT_API)

diff --git a/package.json b/package.json
@@ -95,7 +95,7 @@
     "build.doc": "typedoc --options ./typedoc.json && minify-all -s docs-unminified -d docs --jsCompressor terser && shx rm -rf docs-unminified",
     "deploy.doc": "run-s build.doc && gh-pages --dist \"./docs\"",
     "build.native": "cmake-ts nativeonly",
-    "build.native.debug": "cmake-ts nativeonly",
+    "build.native.debug": "cmake-ts dev-os-only",
     "build": "run-p build.js build.native",
     "build.debug": "run-s build.js build.native.debug",
     "test": "run-s clean.temp build && mocha",
@@ -107,7 +107,7 @@
     "test.electron.renderer": "run-s build && electron-mocha --renderer",
     "lint-test.eslint": "eslint ./**/*.{ts,tsx,js,jsx,cjs,mjs,json,yaml} --no-error-on-unmatched-pattern --cache --cache-location ./.cache/eslint/",
     "lint.eslint": "pnpm run lint-test.eslint --fix",
-    "lint.clang-tidy": "git ls-files --exclude-standard | grep -E '\\.(cpp|hpp|c|cc|cxx|hxx|h|ixx)$' | xargs -n 1 -P $(nproc) clang-tidy --fix ",
+    "lint.clang-tidy": "git ls-files --exclude-standard | grep -E '\\.(cpp|hpp|c|cc|cxx|hxx|h|ixx)$' | xargs -n 1 -P $(nproc) clang-tidy",
     "lint": "run-p format lint.eslint format",
     "lint-test": "run-s lint-test.eslint",
     "bench": "node --expose-gc test/bench",
@@ -124,6 +124,15 @@
         "runtime": "node",
         "runtimeVersion": "12.22.12"
       },
+      {
+        "name": "linux-x64-dev",
+        "dev": true,
+        "buildType": "Debug",
+        "os": "linux",
+        "arch": "x64",
+        "runtime": "node",
+        "runtimeVersion": "12.22.12"
+      },
       {
         "name": "windows-x64",
         "os": "win32",

diff --git a/src/closable.h b/src/closable.h
@@ -4,6 +4,13 @@
    up ZMQ resources at agent exit. */
 namespace zmq {
 struct Closable {
+    Closable() = default;
+    Closable(const Closable&) = default;
+    Closable(Closable&&) = default;
+    Closable& operator=(const Closable&) = default;
+    Closable& operator=(Closable&&) = default;
+    virtual ~Closable() = default;
+
     virtual void Close() = 0;
 };
-}
+}  // namespace zmq
diff --git a/src/context.cc b/src/context.cc
@@ -13,17 +13,21 @@ Context::Context(const Napi::CallbackInfo& info)
     /* If this module has no global context, then create one with a process
        wide context pointer that is shared between threads/agents. */
     if (module.GlobalContext.IsEmpty()) {
-        if (Arg::Validator{}.ThrowIfInvalid(info)) return;
+        if (Arg::Validator{}.ThrowIfInvalid(info)) {
+            return;
+        }
 
         /* Just use the same shared global context pointer. Contexts are
            threadsafe anyway. */
         context = module.Global().SharedContext;
     } else {
-        Arg::Validator args{
+        Arg::Validator const args{
             Arg::Optional<Arg::Object>("Options must be an object"),
         };
 
-        if (args.ThrowIfInvalid(info)) return;
+        if (args.ThrowIfInvalid(info)) {
+            return;
+        }
 
         context = zmq_ctx_new();
     }
@@ -62,7 +66,7 @@ void Context::Close() {
                termination may block depending on ZMQ_BLOCKY/ZMQ_LINGER. This
                should definitely be avoided during GC and may only be acceptable
                at process exit. */
-            auto err = zmq_ctx_shutdown(context);
+            [[maybe_unused]] auto err = zmq_ctx_shutdown(context);
             assert(err == 0);
 
             /* Pass the ZMQ context on to terminator for cleanup at exit. */
@@ -76,35 +80,39 @@ void Context::Close() {
 
 template <>
 Napi::Value Context::GetCtxOpt<bool>(const Napi::CallbackInfo& info) {
-    Arg::Validator args{
+    Arg::Validator const args{
         Arg::Required<Arg::Number>("Identifier must be a number"),
     };
 
-    if (args.ThrowIfInvalid(info)) return Env().Undefined();
+    if (args.ThrowIfInvalid(info)) {
+        return Env().Undefined();
+    }
 
-    uint32_t option = info[0].As<Napi::Number>();
+    const auto option = info[0].As<Napi::Number>();
 
-    int32_t value = zmq_ctx_get(context, option);
+    int32_t const value = zmq_ctx_get(context, option);
     if (value < 0) {
         ErrnoException(Env(), zmq_errno()).ThrowAsJavaScriptException();
         return Env().Undefined();
     }
 
-    return Napi::Boolean::New(Env(), value);
+    return Napi::Boolean::New(Env(), value != 0);
 }
 
 template <>
 void Context::SetCtxOpt<bool>(const Napi::CallbackInfo& info) {
-    Arg::Validator args{
+    Arg::Validator const args{
         Arg::Required<Arg::Number>("Identifier must be a number"),
         Arg::Required<Arg::Boolean>("Option value must be a boolean"),
     };
 
-    if (args.ThrowIfInvalid(info)) return;
+    if (args.ThrowIfInvalid(info)) {
+        return;
+    }
 
-    uint32_t option = info[0].As<Napi::Number>();
+    const auto option = info[0].As<Napi::Number>();
 
-    int32_t value = info[1].As<Napi::Boolean>();
+    int32_t const value = static_cast<int32_t>(info[1].As<Napi::Boolean>());
     if (zmq_ctx_set(context, option, value) < 0) {
         ErrnoException(Env(), zmq_errno()).ThrowAsJavaScriptException();
         return;
@@ -113,13 +121,15 @@ void Context::SetCtxOpt<bool>(const Napi::CallbackInfo& info) {
 
 template <typename T>
 Napi::Value Context::GetCtxOpt(const Napi::CallbackInfo& info) {
-    Arg::Validator args{
+    Arg::Validator const args{
         Arg::Required<Arg::Number>("Identifier must be a number"),
     };
 
-    if (args.ThrowIfInvalid(info)) return Env().Undefined();
+    if (args.ThrowIfInvalid(info)) {
+        return Env().Undefined();
+    }
 
-    uint32_t option = info[0].As<Napi::Number>();
+    const auto option = info[0].As<Napi::Number>();
 
     T value = zmq_ctx_get(context, option);
     if (value < 0) {
@@ -132,14 +142,16 @@ Napi::Value Context::GetCtxOpt(const Napi::CallbackInfo& info) {
 
 template <typename T>
 void Context::SetCtxOpt(const Napi::CallbackInfo& info) {
-    Arg::Validator args{
+    Arg::Validator const args{
         Arg::Required<Arg::Number>("Identifier must be a number"),
         Arg::Required<Arg::Number>("Option value must be a number"),
     };
 
-    if (args.ThrowIfInvalid(info)) return;
+    if (args.ThrowIfInvalid(info)) {
+        return;
+    }
 
-    uint32_t option = info[0].As<Napi::Number>();
+    const auto option = info[0].As<Napi::Number>();
 
     T value = info[1].As<Napi::Number>();
     if (zmq_ctx_set(context, option, value) < 0) {
@@ -166,4 +178,4 @@ void Context::Initialize(Module& module, Napi::Object& exports) {
     module.Context = Napi::Persistent(constructor);
     exports.Set("Context", constructor);
 }
-}
+}  // namespace zmq
diff --git a/src/context.h b/src/context.h
@@ -12,10 +12,12 @@ class Context : public Napi::ObjectWrap<Context>, public Closable {
     static void Initialize(Module& module, Napi::Object& exports);
 
     explicit Context(const Napi::CallbackInfo& info);
-    virtual ~Context();
 
+    Context(const Context&) = delete;
+    Context& operator=(const Context&) = delete;
     Context(Context&&) = delete;
     Context& operator=(Context&&) = delete;
+    ~Context() override;
 
     void Close() override;
 
@@ -34,7 +36,7 @@ class Context : public Napi::ObjectWrap<Context>, public Closable {
     friend class Observer;
     friend class Proxy;
 };
-}
+}  // namespace zmq
 
-static_assert(!std::is_copy_constructible<zmq::Context>::value, "not copyable");
-static_assert(!std::is_move_constructible<zmq::Context>::value, "not movable");
+static_assert(!std::is_copy_constructible_v<zmq::Context>, "not copyable");
+static_assert(!std::is_move_constructible_v<zmq::Context>, "not movable");
diff --git a/src/incoming_msg.cc b/src/incoming_msg.cc
@@ -2,6 +2,7 @@
 #include "./incoming_msg.h"
 
 #include <cassert>
+#include <cstdint>
 
 #include "util/electron_helper.h"
 #include "util/error.h"
@@ -26,44 +27,46 @@ Napi::Value IncomingMsg::IntoBuffer(const Napi::Env& env) {
             return env.Undefined();
         }
     }
-    auto data = reinterpret_cast<uint8_t*>(zmq_msg_data(*ref));
-    auto length = zmq_msg_size(*ref);
+    auto* data = reinterpret_cast<uint8_t*>(zmq_msg_data(ref->get()));
+    auto length = zmq_msg_size(ref->get());
 
     if (noElectronMemoryCage) {
-        static auto constexpr zero_copy_threshold = 1 << 7;
+        static auto constexpr zero_copy_threshold = 1U << 7U;
         if (length > zero_copy_threshold) {
             /* Reuse existing buffer for external storage. This avoids copying but
                does include an overhead in having to call a finalizer when the
                buffer is GC'ed. For very small messages it is faster to copy. */
             moved = true;
 
             /* Put appropriate GC pressure according to the size of the buffer. */
-            Napi::MemoryManagement::AdjustExternalMemory(env, length);
+            Napi::MemoryManagement::AdjustExternalMemory(
+                env, static_cast<int64_t>(length));
 
-            auto release = [](const Napi::Env& env, uint8_t*, Reference* ref) {
-                ptrdiff_t length = zmq_msg_size(*ref);
+            const auto release = [](const Napi::Env& env, uint8_t*, Reference* ref) {
+                const auto length = static_cast<int64_t>(zmq_msg_size(ref->get()));
                 Napi::MemoryManagement::AdjustExternalMemory(env, -length);
                 delete ref;
             };
 
-            return Napi::Buffer<uint8_t>::New(env, data, length, release, ref);
+            return Napi::Buffer<uint8_t>::New(env, data, length, release, ref)
+                .As<Napi::Value>();
         }
     }
 
     if (length > 0) {
-        return Napi::Buffer<uint8_t>::Copy(env, data, length);
+        return Napi::Buffer<uint8_t>::Copy(env, data, length).As<Napi::Value>();
     }
 
-    return Napi::Buffer<uint8_t>::New(env, 0);
+    return Napi::Buffer<uint8_t>::New(env, 0).As<Napi::Value>();
 }
 
 IncomingMsg::Reference::Reference() {
-    auto err = zmq_msg_init(&msg);
+    [[maybe_unused]] auto err = zmq_msg_init(&msg);
     assert(err == 0);
 }
 
 IncomingMsg::Reference::~Reference() {
-    auto err = zmq_msg_close(&msg);
+    [[maybe_unused]] auto err = zmq_msg_close(&msg);
     assert(err == 0);
 }
-}
+}  // namespace zmq
diff --git a/src/incoming_msg.h b/src/incoming_msg.h
@@ -12,30 +12,36 @@ class IncomingMsg {
 
     IncomingMsg(const IncomingMsg&) = delete;
     IncomingMsg& operator=(const IncomingMsg&) = delete;
+    IncomingMsg(IncomingMsg&&) = delete;
+    IncomingMsg& operator=(IncomingMsg&&) = delete;
 
     Napi::Value IntoBuffer(const Napi::Env& env);
 
-    inline operator zmq_msg_t*() {
-        return *ref;
+    zmq_msg_t* get() {
+        return ref->get();
     }
 
 private:
     class Reference {
-        zmq_msg_t msg;
+        zmq_msg_t msg{};
 
     public:
         Reference();
+        Reference(const Reference&) = delete;
+        Reference(Reference&&) = delete;
+        Reference& operator=(const Reference&) = delete;
+        Reference& operator=(Reference&&) = delete;
         ~Reference();
 
-        inline operator zmq_msg_t*() {
+        zmq_msg_t* get() {
             return &msg;
         }
     };
 
     Reference* ref = nullptr;
     bool moved = false;
 };
-}
+}  // namespace zmq
 
-static_assert(!std::is_copy_constructible<zmq::IncomingMsg>::value, "not copyable");
-static_assert(!std::is_move_constructible<zmq::IncomingMsg>::value, "not movable");
+static_assert(!std::is_copy_constructible_v<zmq::IncomingMsg>, "not copyable");
+static_assert(!std::is_move_constructible_v<zmq::IncomingMsg>, "not movable");
diff --git a/src/inline.h b/src/inline.h
@@ -1,7 +1,7 @@
 #pragma once
 
-#ifdef _MSC_VER
-#define force_inline inline __forceinline
+#if defined(_MSC_VER) && !defined(__clang__)
+#define force_inline __forceinline
 #else
 #define force_inline inline __attribute__((always_inline))
 #endif