From 9f7c8654549e8deeb434c9f653e31d6900acdc00 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Mon, 7 Sep 2020 16:19:10 -0500 Subject: [PATCH] refactor: don't store torrent hashes in QStrings (#1428) * refactor: keep torrent hash in std::array This replaces the hashString QString allocation with a compile-time array that's included in sizeof the Torrent struct that owns it. --- qt/DetailsDialog.cc | 23 +++++------ qt/Torrent.cc | 2 +- qt/Torrent.h | 53 +++++++++++++++++++++++--- qt/TorrentFilter.cc | 6 +-- qt/TorrentModel.cc | 4 +- qt/TorrentModel.h | 2 +- qt/VariantHelpers.cc | 7 +++- qt/VariantHelpers.h | 16 ++++++++ qt/WatchDir.cc | 2 +- tests/libtransmission/clients-test.cc | 6 +-- tests/libtransmission/crypto-test.cc | 6 +-- tests/libtransmission/file-test.cc | 6 +-- tests/libtransmission/metainfo-test.cc | 12 +++--- tests/libtransmission/variant-test.cc | 12 +++--- 14 files changed, 108 insertions(+), 49 deletions(-) diff --git a/qt/DetailsDialog.cc b/qt/DetailsDialog.cc index 03c28ece4..316f80821 100644 --- a/qt/DetailsDialog.cc +++ b/qt/DetailsDialog.cc @@ -794,20 +794,17 @@ void DetailsDialog::refreshUI() ui_.sizeValueLabel->setText(string); // myHashLabel - string = none; - - if (!torrents.empty()) + if (torrents.empty()) { - string = torrents[0]->hashString(); - - for (Torrent const* const t : torrents) - { - if (string != t->hashString()) - { - string = mixed; - break; - } - } + string = none; + } + else if (torrents.size() > 1) + { + string = mixed; + } + else + { + string = torrents.front()->hash().toString(); } ui_.hashValueLabel->setText(string); diff --git a/qt/Torrent.cc b/qt/Torrent.cc index 68fdbfe57..87617c76b 100644 --- a/qt/Torrent.cc +++ b/qt/Torrent.cc @@ -222,7 +222,7 @@ Torrent::fields_t Torrent::update(tr_quark const* keys, tr_variant const* const* HANDLE_KEY(eta, eta, ETA) HANDLE_KEY(fileStats, files, FILES) HANDLE_KEY(files, files, FILES) - HANDLE_KEY(hashString, hash_string, HASH_STRING) + HANDLE_KEY(hashString, hash, HASH) HANDLE_KEY(haveUnchecked, have_unchecked, HAVE_UNCHECKED) HANDLE_KEY(haveValid, have_verified, HAVE_VERIFIED) HANDLE_KEY(honorsSessionLimits, honors_session_limits, HONORS_SESSION_LIMITS) diff --git a/qt/Torrent.h b/qt/Torrent.h index f71286800..b56ab155e 100644 --- a/qt/Torrent.h +++ b/qt/Torrent.h @@ -8,6 +8,7 @@ #pragma once +#include #include #include // time_t @@ -17,6 +18,7 @@ #include #include +#include #include #include "FaviconCache.h" @@ -84,7 +86,6 @@ struct TrackerStat int tier; FaviconCache::Key favicon_key; QString announce; - QString host; QString last_announce_result; QString last_scrape_result; }; @@ -103,6 +104,47 @@ struct TorrentFile using FileList = QVector; +class TorrentHash +{ +private: + std::array data_ = {}; + +public: + TorrentHash() {} + + TorrentHash(char const* str) + { + tr_hex_to_sha1(data_.data(), str); + } + + TorrentHash(QString const& str) + { + tr_hex_to_sha1(data_.data(), str.toUtf8().constData()); + } + + bool operator ==(TorrentHash const& that) const + { + return data_ == that.data_; + } + + bool operator !=(TorrentHash const& that) const + { + return data_ != that.data_; + } + + bool operator <(TorrentHash const& that) const + { + return data_ < that.data_; + } + + QString toString() const + { + char str[SHA_DIGEST_LENGTH * 2 + 1]; + tr_sha1_to_hex(str, data_.data()); + return QString::fromUtf8(str, SHA_DIGEST_LENGTH * 2); + } +}; + class Torrent : public QObject { Q_OBJECT @@ -148,9 +190,9 @@ public: QString getError() const; - QString const& hashString() const + TorrentHash const& hash() const { - return hash_string_; + return hash_; } bool hasError() const @@ -523,7 +565,7 @@ public: ETA, FAILED_EVER, FILES, - HASH_STRING, + HASH, HAVE_UNCHECKED, HAVE_VERIFIED, HONORS_SESSION_LIMITS, @@ -622,7 +664,6 @@ private: QString creator_; QString download_dir_; QString error_string_; - QString hash_string_; QString name_; QIcon icon_; @@ -637,6 +678,8 @@ private: Speed download_speed_; Prefs const& prefs_; + + TorrentHash hash_; }; Q_DECLARE_METATYPE(Torrent const*) diff --git a/qt/TorrentFilter.cc b/qt/TorrentFilter.cc index 424255345..d67a0cbf3 100644 --- a/qt/TorrentFilter.cc +++ b/qt/TorrentFilter.cc @@ -221,7 +221,7 @@ bool TorrentFilter::lessThan(QModelIndex const& left, QModelIndex const& right) if (val == 0) { - val = compare(a->hashString(), b->hashString()); + val = compare(a->hash(), b->hash()); } return val < 0; @@ -253,9 +253,7 @@ bool TorrentFilter::filterAcceptsRow(int source_row, QModelIndex const& source_p if (accepts) { auto const text = prefs_.getString(Prefs::FILTER_TEXT); - accepts = text.isEmpty() || - tor.name().contains(text, Qt::CaseInsensitive) || - tor.hashString().contains(text, Qt::CaseInsensitive); + accepts = text.isEmpty() || tor.name().contains(text, Qt::CaseInsensitive); } return accepts; diff --git a/qt/TorrentModel.cc b/qt/TorrentModel.cc index 543ef9eab..855400678 100644 --- a/qt/TorrentModel.cc +++ b/qt/TorrentModel.cc @@ -485,8 +485,8 @@ void TorrentModel::rowsRemove(torrents_t const& torrents) **** ***/ -bool TorrentModel::hasTorrent(QString const& hash_string) const +bool TorrentModel::hasTorrent(TorrentHash const& hash) const { - auto test = [hash_string](auto const& tor) { return tor->hashString() == hash_string; }; + auto test = [hash](auto const& tor) { return tor->hash() == hash; }; return std::any_of(torrents_.cbegin(), torrents_.cend(), test); } diff --git a/qt/TorrentModel.h b/qt/TorrentModel.h index 6472458db..b8dbdf3bd 100644 --- a/qt/TorrentModel.h +++ b/qt/TorrentModel.h @@ -41,7 +41,7 @@ public: virtual ~TorrentModel() override; void clear(); - bool hasTorrent(QString const& hash_string) const; + bool hasTorrent(TorrentHash const& hash) const; Torrent* getTorrentFromId(int id); Torrent const* getTorrentFromId(int id) const; diff --git a/qt/VariantHelpers.cc b/qt/VariantHelpers.cc index 59c85f25f..c8091b49d 100644 --- a/qt/VariantHelpers.cc +++ b/qt/VariantHelpers.cc @@ -39,6 +39,12 @@ bool change(Speed& setme, tr_variant const* variant) return value && change(setme, Speed::fromBps(*value)); } +bool change(TorrentHash& setme, tr_variant const* value) +{ + auto const hash_string = getValue(value); + return hash_string && change(setme, TorrentHash(hash_string->data())); +} + bool change(Peer& setme, tr_variant const* value) { bool changed = false; @@ -130,7 +136,6 @@ bool change(TrackerStat& setme, tr_variant const* value) HANDLE_KEY(downloadCount, download_count) HANDLE_KEY(hasAnnounced, has_announced) HANDLE_KEY(hasScraped, has_scraped) - HANDLE_KEY(host, host) HANDLE_KEY(id, id) HANDLE_KEY(isBackup, is_backup) HANDLE_KEY(lastAnnouncePeerCount, last_announce_peer_count) diff --git a/qt/VariantHelpers.h b/qt/VariantHelpers.h index 79df9ca56..3986b9be3 100644 --- a/qt/VariantHelpers.h +++ b/qt/VariantHelpers.h @@ -19,6 +19,7 @@ class QByteArray; class Speed; +class TorrentHash; struct Peer; struct TorrentFile; struct TrackerStat; @@ -88,6 +89,20 @@ auto getValue(tr_variant const* variant) return ret; } +template>::type* = nullptr> +auto getValue(tr_variant const* variant) +{ + std::optional ret; + char const* str; + size_t len; + if (tr_variantGetStr(variant, &str, &len)) + { + ret = std::string_view(str, len); + } + + return ret; +} + template bool change(T& setme, T const& value) { @@ -105,6 +120,7 @@ bool change(double& setme, double const& value); bool change(Speed& setme, tr_variant const* value); bool change(Peer& setme, tr_variant const* value); bool change(TorrentFile& setme, tr_variant const* value); +bool change(TorrentHash& setme, tr_variant const* value); bool change(TrackerStat& setme, tr_variant const* value); template diff --git a/qt/WatchDir.cc b/qt/WatchDir.cc index 4da67d96c..a16202c83 100644 --- a/qt/WatchDir.cc +++ b/qt/WatchDir.cc @@ -45,7 +45,7 @@ int WatchDir::metainfoTest(QString const& filename) const { ret = ERROR; } - else if (model_.hasTorrent(QString::fromUtf8(inf.hashString))) + else if (model_.hasTorrent(TorrentHash(inf.hashString))) { ret = DUPLICATE; } diff --git a/tests/libtransmission/clients-test.cc b/tests/libtransmission/clients-test.cc index 6a8d00799..164337117 100644 --- a/tests/libtransmission/clients-test.cc +++ b/tests/libtransmission/clients-test.cc @@ -15,14 +15,14 @@ TEST(Client, clientForId) { - struct Test_local + struct LocalTest { char const* peer_id; char const* expected_client; }; - auto constexpr Tests = std::array{ - Test_local{ "-BT791B-", "BitTorrent 7.9.1 (Beta)" }, + auto constexpr Tests = std::array{ + LocalTest{ "-BT791B-", "BitTorrent 7.9.1 (Beta)" }, { "-BT791\0-", "BitTorrent 7.9.1" }, { "-FC1013-", "FileCroc 1.0.1.3" }, { "-FC1013-", "FileCroc 1.0.1.3" }, diff --git a/tests/libtransmission/crypto-test.cc b/tests/libtransmission/crypto-test.cc index 004661334..a12ef3452 100644 --- a/tests/libtransmission/crypto-test.cc +++ b/tests/libtransmission/crypto-test.cc @@ -122,14 +122,14 @@ TEST(Crypto, sha1) TEST(Crypto, ssha1) { - struct Test_local + struct LocalTest { char const* const plain_text; char const* const ssha1; }; - auto constexpr Tests = std::array{ - Test_local{ "test", "{15ad0621b259a84d24dcd4e75b09004e98a3627bAMbyRHJy" }, + auto constexpr Tests = std::array{ + LocalTest{ "test", "{15ad0621b259a84d24dcd4e75b09004e98a3627bAMbyRHJy" }, { "QNY)(*#$B)!_X$B !_B#($^!)*&$%CV!#)&$C!@$(P*)", "{10e2d7acbb104d970514a147cd16d51dfa40fb3c0OSwJtOL" } }; diff --git a/tests/libtransmission/file-test.cc b/tests/libtransmission/file-test.cc index d009cc6dc..987be46a0 100644 --- a/tests/libtransmission/file-test.cc +++ b/tests/libtransmission/file-test.cc @@ -976,15 +976,15 @@ TEST_F(FileTest, pathNativeSeparators) { EXPECT_EQ(nullptr, tr_sys_path_native_separators(nullptr)); - struct Test_local + struct LocalTest { std::string input; std::string expected_output; }; - auto const tests = std::array + auto const tests = std::array { - Test_local{ "", "" }, + LocalTest{ "", "" }, { "a", TR_IF_WIN32("a", "a") }, { "/", TR_IF_WIN32("\\", "/") }, { "/a/b/c", TR_IF_WIN32("\\a\\b\\c", "/a/b/c") }, diff --git a/tests/libtransmission/metainfo-test.cc b/tests/libtransmission/metainfo-test.cc index 479529e6c..2def651c0 100644 --- a/tests/libtransmission/metainfo-test.cc +++ b/tests/libtransmission/metainfo-test.cc @@ -52,15 +52,15 @@ TEST(Metainfo, magnetLink) // FIXME: split these into parameterized tests? TEST(Metainfo, bucket) { - struct Test_local + struct LocalTest { int expected_benc_err; int expected_parse_result; void const* benc; }; - auto constexpr Tests = std::array{ - Test_local{ 0, TR_PARSE_OK, BEFORE_PATH "5:a.txt" AFTER_PATH }, + auto constexpr Tests = std::array{ + LocalTest{ 0, TR_PARSE_OK, BEFORE_PATH "5:a.txt" AFTER_PATH }, /* allow empty components, but not =all= empty components, see bug #5517 */ { 0, TR_PARSE_OK, BEFORE_PATH "0:5:a.txt" AFTER_PATH }, @@ -101,7 +101,7 @@ TEST(Metainfo, bucket) TEST(Metainfo, sanitize) { - struct Test_local + struct LocalTest { char const* str; size_t len; @@ -109,9 +109,9 @@ TEST(Metainfo, sanitize) bool expected_is_adjusted; }; - auto constexpr Tests = std::array{ + auto constexpr Tests = std::array{ // skipped - Test_local{ "", 0, nullptr, false }, + LocalTest{ "", 0, nullptr, false }, { ".", 1, nullptr, false }, { "..", 2, nullptr, true }, { ".....", 5, nullptr, false }, diff --git a/tests/libtransmission/variant-test.cc b/tests/libtransmission/variant-test.cc index 7b01ed987..263ddf749 100644 --- a/tests/libtransmission/variant-test.cc +++ b/tests/libtransmission/variant-test.cc @@ -249,14 +249,14 @@ TEST_F(VariantTest, parse) } TEST_F(VariantTest, bencParseAndReencode) { - struct Test_local + struct LocalTest { std::string benc; bool is_good; }; - auto const tests = std::array{ - Test_local{ "llleee", true }, + auto const tests = std::array{ + LocalTest{ "llleee", true }, { "d3:cow3:moo4:spam4:eggse", true }, { "d4:spaml1:a1:bee", true }, { "d5:greenli1ei2ei3ee4:spamd1:ai123e3:keyi214eee", true }, @@ -343,14 +343,14 @@ TEST_F(VariantTest, bencMalformedIncompleteString) TEST_F(VariantTest, bencToJson) { - struct Test_local + struct LocalTest { std::string benc; std::string expected; }; - auto const tests = std::array{ - Test_local{ "i6e", "6" }, + auto const tests = std::array{ + LocalTest{ "i6e", "6" }, { "d5:helloi1e5:worldi2ee", R"({"hello":1,"world":2})" }, { "d5:helloi1e5:worldi2e3:fooli1ei2ei3eee", R"({"foo":[1,2,3],"hello":1,"world":2})" }, { "d5:helloi1e5:worldi2e3:fooli1ei2ei3ed1:ai0eeee", R"({"foo":[1,2,3,{"a":0}],"hello":1,"world":2})" },