From ac6010557ef507985984bf6388f3fb58763dcb35 Mon Sep 17 00:00:00 2001 From: Mike Gelfand Date: Tue, 15 Nov 2022 20:30:32 +0100 Subject: [PATCH] Fix issues reported by clang-tidy `bugprone` checks (GTK client) (#4183) * Fix `bugprone-unchecked-optional-access` clang-tidy issues * Fix `bugprone-easily-swappable-parameters` clang-tidy issues * Extend clang-tidy configuration --- gtk/.clang-tidy | 2 + gtk/FaviconCache.cc | 11 +++- gtk/MakeDialog.cc | 121 +++++++++++++++++++------------------ gtk/PrefsDialog.cc | 3 +- gtk/Session.cc | 13 ++++ gtk/TorrentCellRenderer.cc | 52 ++++++---------- 6 files changed, 106 insertions(+), 96 deletions(-) diff --git a/gtk/.clang-tidy b/gtk/.clang-tidy index c5b189944..9421c5dcc 100644 --- a/gtk/.clang-tidy +++ b/gtk/.clang-tidy @@ -1,6 +1,8 @@ --- Checks: > -*, + bugprone-*, + -bugprone-narrowing-conversions, cert-*, cppcoreguidelines-*, -cppcoreguidelines-avoid-magic-numbers, diff --git a/gtk/FaviconCache.cc b/gtk/FaviconCache.cc index eee800461..a7afca4c9 100644 --- a/gtk/FaviconCache.cc +++ b/gtk/FaviconCache.cc @@ -141,6 +141,13 @@ void gtr_get_favicon_from_url( Glib::ustring const& url, std::function const&)> const& pixbuf_ready_func) { - auto const host = std::string{ tr_urlParse(url.c_str())->host }; - gtr_get_favicon(session, host, pixbuf_ready_func); + if (auto const parsed_url = tr_urlParse(url.c_str()); parsed_url.has_value()) + { + auto const host = std::string{ parsed_url->host }; + gtr_get_favicon(session, host, pixbuf_ready_func); + } + else + { + pixbuf_ready_func({}); + } } diff --git a/gtk/MakeDialog.cc b/gtk/MakeDialog.cc index bb3c9bcad..3bd34bc76 100644 --- a/gtk/MakeDialog.cc +++ b/gtk/MakeDialog.cc @@ -43,15 +43,20 @@ public: MakeProgressDialog( BaseObjectType* cast_item, Glib::RefPtr const& builder, - Gtk::Window& parent, tr_metainfo_builder& metainfo_builder, std::future future, - std::string_view const& target, + std::string_view target, Glib::RefPtr const& core); ~MakeProgressDialog() override; TR_DISABLE_COPY_MOVE(MakeProgressDialog) + static std::unique_ptr create( + std::string_view target, + tr_metainfo_builder& metainfo_builder, + std::future future, + Glib::RefPtr const& core); + [[nodiscard]] bool success() const { return success_; @@ -108,8 +113,7 @@ private: void setFilename(std::string_view filename); - void makeProgressDialog(std::string_view target, std::future future); - void configurePieceSizeScale(); + void configurePieceSizeScale(uint32_t piece_size); void onPieceSizeUpdated(); private: @@ -251,10 +255,9 @@ void MakeProgressDialog::onProgressDialogResponse(int response) MakeProgressDialog::MakeProgressDialog( BaseObjectType* cast_item, Glib::RefPtr const& builder, - Gtk::Window& parent, tr_metainfo_builder& metainfo_builder, std::future future, - std::string_view const& target, + std::string_view target, Glib::RefPtr const& core) : Gtk::Dialog(cast_item) , builder_(metainfo_builder) @@ -264,7 +267,6 @@ MakeProgressDialog::MakeProgressDialog( , progress_label_(gtr_get_widget(builder, "progress_label")) , progress_bar_(gtr_get_widget(builder, "progress_bar")) { - set_transient_for(parent); signal_response().connect(sigc::mem_fun(*this, &MakeProgressDialog::onProgressDialogResponse)); progress_tag_ = Glib::signal_timeout().connect_seconds( @@ -273,17 +275,62 @@ MakeProgressDialog::MakeProgressDialog( onProgressDialogRefresh(); } -void MakeDialog::Impl::makeProgressDialog(std::string_view target, std::future future) +std::unique_ptr MakeProgressDialog::create( + std::string_view target, + tr_metainfo_builder& metainfo_builder, + std::future future, + Glib::RefPtr const& core) { auto const builder = Gtk::Builder::create_from_resource(gtr_get_full_resource_path("MakeProgressDialog.ui")); - progress_dialog_ = std::unique_ptr(gtr_get_widget_derived( + return std::unique_ptr(gtr_get_widget_derived( builder, "MakeProgressDialog", - dialog_, - *builder_, + metainfo_builder, std::move(future), target, - core_)); + core)); +} + +void MakeDialog::Impl::onResponse(int response) +{ + if (response == TR_GTK_RESPONSE_TYPE(CLOSE)) + { + dialog_.close(); + return; + } + + if (response != TR_GTK_RESPONSE_TYPE(ACCEPT) || !builder_.has_value()) + { + return; + } + + // destination file + auto const dir = destination_chooser_->get_filename(); + auto const base = Glib::path_get_basename(builder_->top()); + auto const target = fmt::format("{:s}/{:s}.torrent", dir, base); + + // build the announce list + auto trackers = tr_announce_list{}; + trackers.parse(announce_text_buffer_->get_text(false).raw()); + builder_->setAnnounceList(std::move(trackers)); + + // comment + if (comment_check_->get_active()) + { + builder_->setComment(comment_entry_->get_text().raw()); + } + + // source + if (source_check_->get_active()) + { + builder_->setSource(source_entry_->get_text().raw()); + } + + builder_->setPrivate(private_check_->get_active()); + + // build the .torrent + progress_dialog_ = MakeProgressDialog::create(target, *builder_, builder_->makeChecksums(), core_); + progress_dialog_->set_transient_for(dialog_); gtr_window_on_close( *progress_dialog_, [this]() @@ -298,57 +345,15 @@ void MakeDialog::Impl::makeProgressDialog(std::string_view target, std::futureshow(); } -void MakeDialog::Impl::onResponse(int response) -{ - if (response == TR_GTK_RESPONSE_TYPE(ACCEPT)) - { - if (builder_) - { - // destination file - auto const dir = destination_chooser_->get_filename(); - auto const base = Glib::path_get_basename(builder_->top()); - auto const target = fmt::format("{:s}/{:s}.torrent", dir, base); - - // build the announce list - auto trackers = tr_announce_list{}; - trackers.parse(announce_text_buffer_->get_text(false).raw()); - builder_->setAnnounceList(std::move(trackers)); - - // comment - if (comment_check_->get_active()) - { - builder_->setComment(comment_entry_->get_text().raw()); - } - - // source - if (source_check_->get_active()) - { - builder_->setSource(source_entry_->get_text().raw()); - } - - builder_->setPrivate(private_check_->get_active()); - - // build the .torrent - makeProgressDialog(target, builder_->makeChecksums()); - } - } - else if (response == TR_GTK_RESPONSE_TYPE(CLOSE)) - { - dialog_.close(); - } -} - /*** **** ***/ void MakeDialog::Impl::updatePiecesLabel() { - auto const filename = builder_ ? builder_->top() : ""sv; - auto gstr = Glib::ustring(); - if (std::empty(filename)) + if (!builder_.has_value() || std::empty(builder_->top())) { gstr += _("No source selected"); piece_size_scale_->set_visible(false); @@ -372,10 +377,10 @@ void MakeDialog::Impl::updatePiecesLabel() pieces_lb_->set_text(gstr); } -void MakeDialog::Impl::configurePieceSizeScale() +void MakeDialog::Impl::configurePieceSizeScale(uint32_t piece_size) { // the below lower & upper bounds would allow piece size selection between approx 1KiB - 64MiB - auto adjustment = Gtk::Adjustment::create(log2(builder_->pieceSize()), 10, 26, 1.0, 1.0); + auto adjustment = Gtk::Adjustment::create(log2(piece_size), 10, 26, 1.0, 1.0); piece_size_scale_->set_adjustment(adjustment); piece_size_scale_->set_visible(true); } @@ -387,7 +392,7 @@ void MakeDialog::Impl::setFilename(std::string_view filename) if (!filename.empty()) { builder_.emplace(filename); - configurePieceSizeScale(); + configurePieceSizeScale(builder_->pieceSize()); } updatePiecesLabel(); diff --git a/gtk/PrefsDialog.cc b/gtk/PrefsDialog.cc index 36a13ee83..d428e9b13 100644 --- a/gtk/PrefsDialog.cc +++ b/gtk/PrefsDialog.cc @@ -725,6 +725,7 @@ void RemotePage::refreshWhitelist() core_->set_pref(TR_KEY_rpc_whitelist, str); } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) void RemotePage::onAddressEdited(Glib::ustring const& path, Glib::ustring const& address) { if (auto const iter = store_->get_iter(path); iter) @@ -836,7 +837,7 @@ RemotePage::RemotePage(BaseObjectType* cast_item, Glib::RefPtr con /* ip address column */ auto* r = Gtk::make_managed(); - r->signal_edited().connect([this](auto const& path, auto const& new_text) { onAddressEdited(path, new_text); }); + r->signal_edited().connect(sigc::mem_fun(*this, &RemotePage::onAddressEdited)); r->property_editable() = true; auto* c = Gtk::make_managed("", *r); c->add_attribute(r->property_text(), whitelist_cols.address); diff --git a/gtk/Session.cc b/gtk/Session.cc index 89bf35301..d941aaf53 100644 --- a/gtk/Session.cc +++ b/gtk/Session.cc @@ -313,6 +313,7 @@ bool is_valid_eta(int t) return t != TR_ETA_NOT_AVAIL && t != TR_ETA_UNKNOWN; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_eta(int a, int b) { bool const a_valid = is_valid_eta(a); @@ -337,11 +338,13 @@ int compare_eta(int a, int b) } template +// 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) { int ret = 0; @@ -366,11 +369,13 @@ int compare_ratio(double a, double b) return ret; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_name(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { return a->get_value(torrent_cols.name_collated).compare(b->get_value(torrent_cols.name_collated)); } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_queue(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { auto const* const sa = tr_torrentStatCached(static_cast(a->get_value(torrent_cols.torrent))); @@ -379,6 +384,7 @@ int compare_by_queue(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::co return sb->queuePosition - sa->queuePosition; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_ratio(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { int ret = 0; @@ -399,6 +405,7 @@ int compare_by_ratio(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::co return ret; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_activity(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { int ret = 0; @@ -427,6 +434,7 @@ int compare_by_activity(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel: return ret; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_age(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { auto* const ta = static_cast(a->get_value(torrent_cols.torrent)); @@ -441,6 +449,7 @@ int compare_by_age(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::cons return ret; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_size(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { auto const size_a = tr_torrentTotalSize(static_cast(a->get_value(torrent_cols.torrent))); @@ -455,6 +464,7 @@ int compare_by_size(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::con return ret; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_progress(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { auto const* const sa = tr_torrentStatCached(static_cast(a->get_value(torrent_cols.torrent))); @@ -474,6 +484,7 @@ int compare_by_progress(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel: return ret; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_eta(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { auto const* const sa = tr_torrentStatCached(static_cast(a->get_value(torrent_cols.torrent))); @@ -488,6 +499,7 @@ int compare_by_eta(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::cons return ret; } +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int compare_by_state(Gtk::TreeModel::const_iterator const& a, Gtk::TreeModel::const_iterator const& b) { auto const sa = a->get_value(torrent_cols.activity); @@ -1283,6 +1295,7 @@ void Session::clear() namespace { +// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) int gtr_compare_double(double const a, double const b, int decimal_places) { auto const ia = int64_t(a * pow(10, decimal_places)); diff --git a/gtk/TorrentCellRenderer.cc b/gtk/TorrentCellRenderer.cc index 77655e70b..9ae974443 100644 --- a/gtk/TorrentCellRenderer.cc +++ b/gtk/TorrentCellRenderer.cc @@ -324,8 +324,8 @@ public: TR_DISABLE_COPY_MOVE(Impl) - void get_size_compact(Gtk::Widget& widget, int& width, int& height) const; - void get_size_full(Gtk::Widget& widget, int& width, int& height) const; + Gtk::Requisition get_size_compact(Gtk::Widget& widget) const; + Gtk::Requisition get_size_full(Gtk::Widget& widget) const; void render_compact( SnapshotPtr const& snapshot, @@ -442,7 +442,7 @@ void TorrentCellRenderer::Impl::set_icon( #endif } -void TorrentCellRenderer::Impl::get_size_compact(Gtk::Widget& widget, int& width, int& height) const +Gtk::Requisition TorrentCellRenderer::Impl::get_size_compact(Gtk::Widget& widget) const { int xpad = 0; int ypad = 0; @@ -478,11 +478,11 @@ void TorrentCellRenderer::Impl::get_size_compact(Gtk::Widget& widget, int& width *** LAYOUT **/ - width = xpad * 2 + get_width(icon_size) + GUI_PAD + CompactBarWidth + GUI_PAD + get_width(stat_size); - height = ypad * 2 + std::max(get_height(name_size), property_bar_height_.get_value()); + return { xpad * 2 + get_width(icon_size) + GUI_PAD + CompactBarWidth + GUI_PAD + get_width(stat_size), + ypad * 2 + std::max(get_height(name_size), property_bar_height_.get_value()) }; } -void TorrentCellRenderer::Impl::get_size_full(Gtk::Widget& widget, int& width, int& height) const +Gtk::Requisition TorrentCellRenderer::Impl::get_size_full(Gtk::Widget& widget) const { int xpad = 0; int ypad = 0; @@ -526,29 +526,20 @@ void TorrentCellRenderer::Impl::get_size_full(Gtk::Widget& widget, int& width, i *** LAYOUT **/ - width = xpad * 2 + get_width(icon_size) + GUI_PAD + std::max(get_width(prog_size), get_width(stat_size)); - height = ypad * 2 + get_height(name_size) + get_height(prog_size) + GUI_PAD_SMALL + property_bar_height_.get_value() + - GUI_PAD_SMALL + get_height(stat_size); + return { xpad * 2 + get_width(icon_size) + GUI_PAD + std::max(get_width(prog_size), get_width(stat_size)), + ypad * 2 + get_height(name_size) + get_height(prog_size) + GUI_PAD_SMALL + property_bar_height_.get_value() + + GUI_PAD_SMALL + get_height(stat_size) }; } void TorrentCellRenderer::get_preferred_width_vfunc(Gtk::Widget& widget, int& minimum_width, int& natural_width) const { if (impl_->property_torrent().get_value() != nullptr) { - int w = 0; - int h = 0; + auto const size = impl_->property_compact().get_value() ? impl_->get_size_compact(widget) : + impl_->get_size_full(widget); - if (impl_->property_compact().get_value()) - { - impl_->get_size_compact(widget, w, h); - } - else - { - impl_->get_size_full(widget, w, h); - } - - minimum_width = w; - natural_width = w; + minimum_width = get_width(size); + natural_width = minimum_width; } } @@ -556,20 +547,11 @@ void TorrentCellRenderer::get_preferred_height_vfunc(Gtk::Widget& widget, int& m { if (impl_->property_torrent().get_value() != nullptr) { - int w = 0; - int h = 0; + auto const size = impl_->property_compact().get_value() ? impl_->get_size_compact(widget) : + impl_->get_size_full(widget); - if (impl_->property_compact().get_value()) - { - impl_->get_size_compact(widget, w, h); - } - else - { - impl_->get_size_full(widget, w, h); - } - - minimum_height = h; - natural_height = h; + minimum_height = get_height(size); + natural_height = minimum_height; } }