From cf000a2eba6d3913cc765df9988ab060f6026c98 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 24 Jan 2023 11:18:58 +0200 Subject: Get rid of polymorphism/unique_ptr in system_package_status --- bpkg/system-package-manager-debian.cxx | 161 ++++++++++++++++----------------- bpkg/system-package-manager-debian.hxx | 10 +- bpkg/system-package-manager.cxx | 6 -- bpkg/system-package-manager.hxx | 9 +- 4 files changed, 82 insertions(+), 104 deletions(-) diff --git a/bpkg/system-package-manager-debian.cxx b/bpkg/system-package-manager-debian.cxx index adafd32..660fd2d 100644 --- a/bpkg/system-package-manager-debian.cxx +++ b/bpkg/system-package-manager-debian.cxx @@ -45,7 +45,7 @@ namespace bpkg // main package) as well as -doc and -dbg unless requested (the // extra_{doc,dbg} arguments). // - static unique_ptr + static package_status_debian parse_debian_name (const string& nv, bool extra_doc, bool extra_dbg) @@ -72,7 +72,7 @@ namespace bpkg if (ns.empty ()) fail << "empty package group"; - unique_ptr r; + package_status_debian r; // Handle the dev instead of main special case for libraries. // @@ -87,10 +87,10 @@ namespace bpkg suffix (m, "-dev") && !(ns.size () > 1 && suffix (ns[1], "-dev"))) { - r.reset (new package_status_debian ("", move (m))); + r = package_status_debian ("", move (m)); } else - r.reset (new package_status_debian (move (m))); + r = package_status_debian (move (m)); } // Handle the rest. @@ -100,10 +100,10 @@ namespace bpkg string& n (ns[i]); const char* w; - if (string* v = (suffix (n, (w = "-dev")) ? &r->dev : - suffix (n, (w = "-doc")) ? &r->doc : - suffix (n, (w = "-dbg")) ? &r->dbg : - suffix (n, (w = "-common")) ? &r->common : nullptr)) + if (string* v = (suffix (n, (w = "-dev")) ? &r.dev : + suffix (n, (w = "-doc")) ? &r.doc : + suffix (n, (w = "-dbg")) ? &r.dbg : + suffix (n, (w = "-common")) ? &r.common : nullptr)) { if (!v->empty ()) fail << "multiple " << w << " package names in '" << g << "'" << @@ -112,7 +112,7 @@ namespace bpkg *v = move (n); } else - r->extras.push_back (move (n)); + r.extras.push_back (move (n)); } return r; @@ -121,24 +121,24 @@ namespace bpkg strings gs (split (nv, ',')); assert (!gs.empty ()); // *-name value cannot be empty. - unique_ptr r; + package_status_debian r; for (size_t i (0); i != gs.size (); ++i) { if (i == 0) // Main group. r = parse_group (gs[i]); else { - unique_ptr g (parse_group (gs[i])); - - if (!g->main.empty ()) r->extras.push_back (move (g->main)); - if (!g->dev.empty ()) r->extras.push_back (move (g->dev)); - if (!g->doc.empty () && extra_doc) r->extras.push_back (move (g->doc)); - if (!g->dbg.empty () && extra_dbg) r->extras.push_back (move (g->dbg)); - if (!g->common.empty () && false) r->extras.push_back (move (g->common)); - if (!g->extras.empty ()) r->extras.insert ( - r->extras.end (), - make_move_iterator (g->extras.begin ()), - make_move_iterator (g->extras.end ())); + package_status_debian g (parse_group (gs[i])); + + if (!g.main.empty ()) r.extras.push_back (move (g.main)); + if (!g.dev.empty ()) r.extras.push_back (move (g.dev)); + if (!g.doc.empty () && extra_doc) r.extras.push_back (move (g.doc)); + if (!g.dbg.empty () && extra_dbg) r.extras.push_back (move (g.dbg)); + if (!g.common.empty () && false) r.extras.push_back (move (g.common)); + if (!g.extras.empty ()) r.extras.insert ( + r.extras.end (), + make_move_iterator (g.extras.begin ()), + make_move_iterator (g.extras.end ())); } } @@ -803,13 +803,13 @@ namespace bpkg auto i (status_cache_.find (pn)); if (i != status_cache_.end ()) - return i->second.get (); + return i->second ? &*i->second : nullptr; if (aps == nullptr) - return nullptr; + return nullopt; } - vector> candidates; + vector candidates; // Translate our package name to the Debian package names. // @@ -836,19 +836,15 @@ namespace bpkg // non-libraries with the lib prefix (both of which we do not // recomment) will have to provide a manual mapping. // - unique_ptr s; - if (n.compare (0, 3, "lib") == 0) { // Keep the main package name empty as an indication that it is to // be discovered. // - s.reset (new package_status_debian ("", n + "-dev")); + candidates.push_back (package_status_debian ("", n + "-dev")); } else - s.reset (new package_status_debian (n)); - - candidates.push_back (move (s)); + candidates.push_back (package_status_debian (n)); } else { @@ -856,18 +852,17 @@ namespace bpkg // for (const string& n: ns) { - unique_ptr s ( - parse_debian_name (n, need_doc, need_dbg)); + package_status_debian s (parse_debian_name (n, need_doc, need_dbg)); // Suppress duplicates for good measure based on the main package // name (and falling back to -dev if empty). // auto i (find_if (candidates.begin (), candidates.end (), - [&s] (const unique_ptr& x) + [&s] (const package_status_debian& x) { - return s->main.empty () - ? s->dev == x->dev - : s->main == x->main; + return s.main.empty () + ? s.dev == x.dev + : s.main == x.main; })); if (i == candidates.end ()) candidates.push_back (move (s)); @@ -932,25 +927,25 @@ namespace bpkg // First look for an already fully installed package. // - unique_ptr r; + optional r; - for (unique_ptr& ps: candidates) + for (package_status_debian& ps: candidates) { - vector& pps (ps->package_policies); + vector& pps (ps.package_policies); - if (!ps->main.empty ()) pps.emplace_back (ps->main); - if (!ps->dev.empty ()) pps.emplace_back (ps->dev); - if (!ps->doc.empty () && need_doc) pps.emplace_back (ps->doc); - if (!ps->dbg.empty () && need_dbg) pps.emplace_back (ps->dbg); - if (!ps->common.empty () && false) pps.emplace_back (ps->common); - ps->package_policies_main = pps.size (); - for (const string& n: ps->extras) pps.emplace_back (n); + if (!ps.main.empty ()) pps.emplace_back (ps.main); + if (!ps.dev.empty ()) pps.emplace_back (ps.dev); + if (!ps.doc.empty () && need_doc) pps.emplace_back (ps.doc); + if (!ps.dbg.empty () && need_dbg) pps.emplace_back (ps.dbg); + if (!ps.common.empty () && false) pps.emplace_back (ps.common); + ps.package_policies_main = pps.size (); + for (const string& n: ps.extras) pps.emplace_back (n); apt_cache_policy (pps); // Handle the unknown main package. // - if (ps->main.empty ()) + if (ps.main.empty ()) { const package_policy& dev (pps.front ()); @@ -960,13 +955,13 @@ namespace bpkg if (dev.installed_version.empty ()) continue; - guess_main (*ps, dev.installed_version); - pps.emplace (pps.begin (), ps->main); - ps->package_policies_main++; + guess_main (ps, dev.installed_version); + pps.emplace (pps.begin (), ps.main); + ps.package_policies_main++; apt_cache_policy (pps, 1); } - optional s (status (pps, ps->package_policies_main)); + optional s (status (pps, ps.package_policies_main)); if (!s) continue; @@ -975,15 +970,15 @@ namespace bpkg { const package_policy& main (pps.front ()); - ps->status = *s; - ps->system_name = main.name; - ps->system_version = main.installed_version; + ps.status = *s; + ps.system_name = main.name; + ps.system_version = main.installed_version; - if (r != nullptr) + if (r) { fail << "multiple installed Debian packages for " << pn << info << "first package: " << r->main << " " << r->system_version << - info << "second package: " << ps->main << " " << ps->system_version << + info << "second package: " << ps.main << " " << ps.system_version << info << "consider specifying the desired version manually"; } @@ -993,7 +988,7 @@ namespace bpkg // Next look for available versions if we are allowed to install. // - if (r == nullptr && install) + if (!r && install) { // If we weren't instructed to fetch or we already fetched, then we // don't need to re-run apt_cache_policy(). @@ -1005,38 +1000,35 @@ namespace bpkg fetched_ = true; } - for (unique_ptr& ps: candidates) + for (package_status_debian& ps: candidates) { - vector& pps (ps->package_policies); + vector& pps (ps.package_policies); if (requery) apt_cache_policy (pps); // Handle the unknown main package. // - if (ps->main.empty ()) + if (ps.main.empty ()) { const package_policy& dev (pps.front ()); // Note that this time we use the candidate version. // if (dev.candidate_version.empty ()) - { - ps = nullptr; // Not installable. - continue; - } + continue; // Not installable. - guess_main (*ps, dev.candidate_version); - pps.emplace (pps.begin (), ps->main); - ps->package_policies_main++; + guess_main (ps, dev.candidate_version); + pps.emplace (pps.begin (), ps.main); + ps.package_policies_main++; apt_cache_policy (pps, 1); } - optional s (status (pps, ps->package_policies_main)); + optional s (status (pps, ps.package_policies_main)); if (!s) { - ps = nullptr; // Not installable. + ps.main.clear (); // Not installable. continue; } @@ -1051,21 +1043,21 @@ namespace bpkg // be good enough (especially if we are installing the -dev package) // and there is no straightforward way to change our mind. // - ps->status = *s; - ps->system_name = main.name; - ps->system_version = main.candidate_version; + ps.status = *s; + ps.system_name = main.name; + ps.system_version = main.candidate_version; // Prefer partially installed to not installed. This makes detecting // ambiguity a bit trickier so we handle partially installed here and // not installed in a separate loop below. // - if (ps->status == package_status::partially_installed) + if (ps.status == package_status::partially_installed) { - if (r != nullptr) + if (r) { fail << "multiple partially installed Debian packages for " << pn << info << "first package: " << r->main << " " << r->system_version << - info << "second package: " << ps->main << " " << ps->system_version << + info << "second package: " << ps.main << " " << ps.system_version << info << "consider specifying the desired version manually"; } @@ -1073,20 +1065,20 @@ namespace bpkg } } - if (r == nullptr) + if (!r) { - for (unique_ptr& ps: candidates) + for (package_status_debian& ps: candidates) { - if (ps == nullptr) + if (ps.main.empty ()) continue; - assert (ps->status != package_status::not_installed); // Sanity check. + assert (ps.status != package_status::not_installed); // Sanity check. - if (r != nullptr) + if (r) { fail << "multiple available Debian packages for " << pn << info << "first package: " << r->main << " " << r->system_version << - info << "second package: " << ps->main << " " << ps->system_version << + info << "second package: " << ps.main << " " << ps.system_version << info << "consider installing the desired package manually"; } @@ -1095,7 +1087,7 @@ namespace bpkg } } - if (r != nullptr) + if (r) { // Map the system version to the bpkg version. // @@ -1130,7 +1122,8 @@ namespace bpkg // Cache. // - return status_cache_.emplace (pn, move (r)).first->second.get (); + auto i (status_cache_.emplace (pn, move (r)).first); + return i->second ? &*i->second : nullptr; } void system_package_manager_debian:: @@ -1154,7 +1147,7 @@ namespace bpkg for (const package_name& pn: pns) { auto it (status_cache_.find (pn)); - assert (it != status_cache_.end () && it->second != nullptr); + assert (it != status_cache_.end () && it->second); const package_status_debian& ps (*it->second); diff --git a/bpkg/system-package-manager-debian.hxx b/bpkg/system-package-manager-debian.hxx index e206243..4c17413 100644 --- a/bpkg/system-package-manager-debian.hxx +++ b/bpkg/system-package-manager-debian.hxx @@ -46,9 +46,8 @@ namespace bpkg // For executable packages there is normally no -dev packages but -dbg, // -doc, and -common are plausible. // - class system_package_status_debian: public system_package_status + struct system_package_status_debian: system_package_status { - public: string main; string dev; string doc; @@ -78,6 +77,8 @@ namespace bpkg { assert (!main.empty () || !dev.empty ()); } + + system_package_status_debian () = default; }; class system_package_manager_debian: public system_package_manager @@ -105,10 +106,7 @@ namespace bpkg bool fetched_ = false; // True if already fetched metadata. bool installed_ = false; // True if already installed. - // @@ Don't need unique_ptr/polymorphism. - // - std::map> status_cache_; + std::map> status_cache_; }; } diff --git a/bpkg/system-package-manager.cxx b/bpkg/system-package-manager.cxx index 8af52a0..670cae6 100644 --- a/bpkg/system-package-manager.cxx +++ b/bpkg/system-package-manager.cxx @@ -20,12 +20,6 @@ using namespace butl; namespace bpkg { - system_package_status:: - ~system_package_status () - { - // vtable - } - system_package_manager:: ~system_package_manager () { diff --git a/bpkg/system-package-manager.hxx b/bpkg/system-package-manager.hxx index d91bce5..cfe5576 100644 --- a/bpkg/system-package-manager.hxx +++ b/bpkg/system-package-manager.hxx @@ -79,9 +79,8 @@ namespace bpkg // So for now we keep it simple and pick a single available version but can // probably revise this decision later. // - class system_package_status + struct system_package_status { - public: // Downstream (as in, bpkg package) version. // bpkg::version version; @@ -103,12 +102,6 @@ namespace bpkg enum status_type {installed, partially_installed, not_installed}; status_type status = not_installed; - - public: - virtual - ~system_package_status (); - - system_package_status () = default; }; class system_package_manager -- cgit v1.1