fix: coverity warnings, sonarcloud code smells (#4232)

gtk/FaviconCache.cc

@@ -94,14 +94,11 @@ bool favicon_web_done_idle_cb(std::unique_ptr<favicon_data> fav)
         auto* const session = fav->session;
         auto const next_url = get_url(fav->host, fav->type);
         tr_sessionFetch(session, { next_url.raw(), favicon_web_done_cb, fav.release() });
+        return false;
     }
 
     // Not released into the next web request, means we're done trying (even if `pixbuf` is still invalid)
-    if (fav != nullptr)
-    {
     fav->func(pixbuf);
-    }
-
     return false;
 }
 

gtk/Session.cc

@@ -308,13 +308,30 @@ void Session::Impl::dec_busy()
 namespace
 {
 
-bool is_valid_eta(int t)
+template<typename T>
+// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
+constexpr int compare_generic(T const& a, T const& b)
+{
+    if (a < b)
+    {
+        return -1;
+    }
+
+    if (a > b)
+    {
+        return 1;
+    }
+
+    return 0;
+}
+
+constexpr bool is_valid_eta(time_t t)
 {
     return t != TR_ETA_NOT_AVAIL && t != TR_ETA_UNKNOWN;
 }
 
 // NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
-int compare_eta(int a, int b)
+constexpr int compare_eta(time_t a, time_t b)
 {
     bool const a_valid = is_valid_eta(a);
     bool const b_valid = is_valid_eta(b);
@@ -334,39 +351,28 @@ int compare_eta(int a, int b)
         return 1;
     }
 
-    return a < b ? 1 : -1;
+    return -compare_generic(a, b);
-}
-
-template<typename T>
-// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
-int compare_generic(T&& a, T&& b)
-{
-    return a < b ? -1 : (a > b ? 1 : 0);
 }
 
 // NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
-int compare_ratio(double a, double b)
+constexpr int compare_ratio(double a, double b)
 {
-    int ret = 0;
+    if (static_cast<int>(a) == TR_RATIO_INF && static_cast<int>(b) == TR_RATIO_INF)
-
-    if ((int)a == TR_RATIO_INF && (int)b == TR_RATIO_INF)
     {
-        ret = 0;
+        return 0;
-    }
-    else if ((int)a == TR_RATIO_INF)
-    {
-        ret = 1;
-    }
-    else if ((int)b == TR_RATIO_INF)
-    {
-        ret = -1;
-    }
-    else
-    {
-        ret = compare_generic(a, b);
     }
 
-    return ret;
+    if (static_cast<int>(a) == TR_RATIO_INF)
+    {
+        return 1;
+    }
+
+    if (static_cast<int>(b) == TR_RATIO_INF)
+    {
+        return -1;
+    }
+
+    return compare_generic(a, b);
 }
 
 // NOLINTNEXTLINE(bugprone-easily-swappable-parameters)

libtransmission/announcer-http.cc

@@ -625,7 +625,7 @@ public:
         return log_name_;
     }
 
-    void invoke_callback()
+    void invoke_callback() const
     {
         if (response_func_)
         {

libtransmission/handshake.cc

@@ -256,21 +256,21 @@ static bool buildHandshakeMessage(tr_handshake const* const handshake, uint8_t*
 
 static ReadState tr_handshakeDone(tr_handshake* handshake, bool is_connected);
 
-enum handshake_parse_err_t
+enum class ParseResult
 {
-    HANDSHAKE_OK,
+    Ok,
-    HANDSHAKE_ENCRYPTION_WRONG,
+    EncryptionWrong,
-    HANDSHAKE_BAD_TORRENT,
+    BadTorrent,
-    HANDSHAKE_PEER_IS_SELF,
+    PeerIsSelf,
 };
 
-static handshake_parse_err_t parseHandshake(tr_handshake* handshake, tr_peerIo* peer_io)
+static ParseResult parseHandshake(tr_handshake* handshake, tr_peerIo* peer_io)
 {
     tr_logAddTraceHand(handshake, fmt::format("payload: need {}, got {}", HandshakeSize, peer_io->readBufferSize()));
 
     if (peer_io->readBufferSize() < HandshakeSize)
     {
-        return HANDSHAKE_ENCRYPTION_WRONG;
+        return ParseResult::EncryptionWrong;
     }
 
     /* confirm the protocol */
@@ -278,7 +278,7 @@ static handshake_parse_err_t parseHandshake(tr_handshake* handshake, tr_peerIo*
     peer_io->readBytes(std::data(name), std::size(name));
     if (name != HandshakeName)
     {
-        return HANDSHAKE_ENCRYPTION_WRONG;
+        return ParseResult::EncryptionWrong;
     }
 
     /* read the reserved bytes */
@@ -291,7 +291,7 @@ static handshake_parse_err_t parseHandshake(tr_handshake* handshake, tr_peerIo*
     if (info_hash == tr_sha1_digest_t{} || info_hash != peer_io->torrentHash())
     {
         tr_logAddTraceHand(handshake, "peer returned the wrong hash. wtf?");
-        return HANDSHAKE_BAD_TORRENT;
+        return ParseResult::BadTorrent;
     }
 
     // peer_id
@@ -306,7 +306,7 @@ static handshake_parse_err_t parseHandshake(tr_handshake* handshake, tr_peerIo*
     if (auto const info = handshake->mediator->torrentInfo(info_hash); info && info->client_peer_id == peer_id)
     {
         tr_logAddTraceHand(handshake, "streuth!  we've connected to ourselves.");
-        return HANDSHAKE_PEER_IS_SELF;
+        return ParseResult::PeerIsSelf;
     }
 
     /**
@@ -317,7 +317,7 @@ static handshake_parse_err_t parseHandshake(tr_handshake* handshake, tr_peerIo*
     peer_io->enableLTEP(HANDSHAKE_HAS_LTEP(reserved));
     peer_io->enableFEXT(HANDSHAKE_HAS_FASTEXT(reserved));
 
-    return HANDSHAKE_OK;
+    return ParseResult::Ok;
 }
 
 /***
@@ -906,7 +906,7 @@ static ReadState readPayloadStream(tr_handshake* handshake, tr_peerIo* peer_io)
     auto const i = parseHandshake(handshake, peer_io);
     tr_logAddTraceHand(handshake, fmt::format("parseHandshake returned {}", static_cast<int>(i)));
 
-    if (i != HANDSHAKE_OK)
+    if (i != ParseResult::Ok)
     {
         return tr_handshakeDone(handshake, false);
     }

libtransmission/peer-msgs.cc

@@ -1767,15 +1767,17 @@ static int clientGotBlock(
     TR_ASSERT(msgs != nullptr);
 
     tr_torrent* const tor = msgs->torrent;
+    auto const n_expected = msgs->torrent->blockSize(block);
 
-    if (!block_data || std::size(*block_data) != msgs->torrent->blockSize(block))
+    if (!block_data)
     {
-        logdbg(
+        logdbg(msgs, fmt::format("wrong block size: expected {:d}, got {:d}", n_expected, 0));
-            msgs,
+        return EMSGSIZE;
-            fmt::format(
+    }
-                FMT_STRING("wrong block size -- expected {:d}, got {:d}"),
+
-                msgs->torrent->blockSize(block),
+    if (std::size(*block_data) != msgs->torrent->blockSize(block))
-                block_data ? std::size(*block_data) : 0U));
+    {
+        logdbg(msgs, fmt::format("wrong block size: expected {:d}, got {:d}", n_expected, std::size(*block_data)));
         block_data->clear();
         return EMSGSIZE;
     }

libtransmission/rpc-server.cc

@@ -230,7 +230,7 @@ static void serve_file(struct evhttp_request* req, tr_rpc_server const* server,
     evbuffer_free(response);
 }
 
-static void handle_web_client(struct evhttp_request* req, tr_rpc_server* server)
+static void handle_web_client(struct evhttp_request* req, tr_rpc_server const* server)
 {
     if (std::empty(server->web_client_dir_))
     {
@@ -716,7 +716,7 @@ static void startServer(tr_rpc_server* server)
     else
     {
         evhttp_set_gencb(httpd, handle_request, server);
-        server->httpd = std::unique_ptr<evhttp, void (*)(evhttp*)>{ httpd, evhttp_free };
+        server->httpd = std::unique_ptr<evhttp, void (*)(evhttp*)>{ httpd, &evhttp_free };
 
         tr_logAddInfo(fmt::format(_("Listening for RPC and Web requests on '{address}'"), fmt::arg("address", addr_port_str)));
     }

libtransmission/session.cc

@@ -2169,7 +2169,7 @@ tr_session::tr_session(std::string_view config_dir, tr_variant* settings_dict)
     , timer_maker_{ std::make_unique<libtransmission::EvTimerMaker>(eventBase()) }
     , settings_{ settings_dict }
     , session_id_{ tr_time }
-    , peer_mgr_{ tr_peerMgrNew(this), tr_peerMgrFree }
+    , peer_mgr_{ tr_peerMgrNew(this), &tr_peerMgrFree }
     , rpc_server_{ std::make_unique<tr_rpc_server>(this, settings_dict) }
 {
     now_timer_ = timerMaker().create([this]() { onNowTimer(); });

libtransmission/verify.cc

@@ -56,7 +56,7 @@ int tr_verify_worker::Node::compare(tr_verify_worker::Node const& that) const
     return 0;
 }
 
-bool tr_verify_worker::verifyTorrent(tr_torrent* tor, std::atomic<bool>& stop_flag)
+bool tr_verify_worker::verifyTorrent(tr_torrent* tor, std::atomic<bool> const& stop_flag)
 {
     auto const begin = tr_time();
 

libtransmission/verify.h

@@ -61,7 +61,7 @@ private:
     }
 
     void verifyThreadFunc();
-    [[nodiscard]] static bool verifyTorrent(tr_torrent* tor, std::atomic<bool>& stop_flag);
+    [[nodiscard]] static bool verifyTorrent(tr_torrent* tor, std::atomic<bool> const& stop_flag);
 
     std::list<callback_func> callbacks_;
     std::mutex verify_mutex_;

qt/FileTreeItem.cc

@@ -85,7 +85,7 @@ int FileTreeItem::row() const
     if (parent_ != nullptr)
     {
         i = parent_->getMyChildRows().value(name(), -1);
-        assert(this == parent_->children_[i]);
+        assert(i == -1 || this == parent_->children_[i]);
     }
 
     return i;

tests/libtransmission/file-test.cc

@@ -1030,26 +1030,18 @@ TEST_F(FileTest, pathNativeSeparators)
 {
     EXPECT_EQ(nullptr, tr_sys_path_native_separators(nullptr));
 
-    struct LocalTest
+    static auto constexpr Tests = std::array<std::pair<std::string_view, std::string_view>, 5>{ {
-    {
+        { "", "" },
-        std::string input;
-        std::string expected_output;
-    };
-
-    auto const tests = std::array<LocalTest, 5>{
-        LocalTest{ "", "" },
         { "a", TR_IF_WIN32("a", "a") },
         { "/", TR_IF_WIN32("\\", "/") },
         { "/a/b/c", TR_IF_WIN32("\\a\\b\\c", "/a/b/c") },
         { "C:\\a/b\\c", TR_IF_WIN32("C:\\a\\b\\c", "C:\\a/b\\c") },
-    };
+    } };
 
-    for (auto const& test : tests)
+    for (auto const& [input, expected] : Tests)
     {
-        auto buf = std::string(test.input);
+        auto buf = tr_pathbuf{ input };
-        char* const output = tr_sys_path_native_separators(buf.data());
+        EXPECT_EQ(expected, tr_sys_path_native_separators(std::data(buf)));
-        EXPECT_EQ(test.expected_output, output);
-        EXPECT_EQ(buf.data(), output);
     }
 }