From e39dea3d68da1b90b657d51b11cacdaf51482a8f Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Sat, 11 May 2024 11:47:45 +0200 Subject: [PATCH] core: replace divesite_table_t by a vector of std::unique_ptr<>s This is a long commit, because it introduces a new abstraction: a general std::vector<> of std::unique_ptrs<>. Moreover, it replaces a number of pointers by C++ references, when the callee does not suppoert null objects. This simplifies memory management and makes ownership more explicit. It is a proof-of-concept and a test-bed for the other core data structrures. Signed-off-by: Berthold Stoeger --- backend-shared/exportfuncs.cpp | 15 +- commands/command.cpp | 4 +- commands/command.h | 2 +- commands/command_divelist.cpp | 21 +-- commands/command_divesite.cpp | 36 ++-- commands/command_divesite.h | 4 +- commands/command_edit.cpp | 24 +-- commands/command_pictures.cpp | 12 +- core/datatrak.cpp | 4 +- core/dive.h | 1 - core/divelist.cpp | 41 ++--- core/divelist.h | 4 +- core/divelog.cpp | 11 +- core/divelog.h | 4 +- core/divesite.cpp | 204 +++++++---------------- core/divesite.h | 60 +++---- core/import-cobalt.cpp | 2 +- core/import-divinglog.cpp | 2 +- core/libdivecomputer.cpp | 2 +- core/liquivision.cpp | 4 +- core/load-git.cpp | 12 +- core/owning_table.h | 153 +++++++++++++++++ core/parse-xml.cpp | 16 +- core/parse.cpp | 10 +- core/save-git.cpp | 5 +- core/save-xml.cpp | 5 +- core/uemis-downloader.cpp | 17 +- desktop-widgets/divesiteimportdialog.cpp | 41 ++--- desktop-widgets/divesiteimportdialog.h | 7 +- desktop-widgets/divesitelistview.cpp | 2 +- desktop-widgets/locationinformation.cpp | 21 +-- desktop-widgets/mainwindow.cpp | 6 +- mobile-widgets/qmlmanager.cpp | 4 +- qt-models/divelocationmodel.cpp | 59 +++---- qt-models/divelocationmodel.h | 2 +- qt-models/divesiteimportmodel.cpp | 21 +-- qt-models/divesiteimportmodel.h | 4 +- qt-models/maplocationmodel.cpp | 25 ++- smtk-import/smartrak.cpp | 6 +- tests/testdivesiteduplication.cpp | 2 +- tests/testparse.cpp | 2 +- 41 files changed, 451 insertions(+), 426 deletions(-) create mode 100644 core/owning_table.h diff --git a/backend-shared/exportfuncs.cpp b/backend-shared/exportfuncs.cpp index a24fba7a8..ff98041d3 100644 --- a/backend-shared/exportfuncs.cpp +++ b/backend-shared/exportfuncs.cpp @@ -325,21 +325,18 @@ std::vector getDiveSitesToExport(bool selectedOnly) return res; } - res.reserve(divelog.sites->nr); - for (int i = 0; i < divelog.sites->nr; i++) { - struct dive_site *ds = get_dive_site(i, divelog.sites); - if (dive_site_is_empty(ds)) + res.reserve(divelog.sites->size()); + for (const auto &ds: *divelog.sites) { + if (dive_site_is_empty(ds.get())) continue; if (selectedOnly && !is_dive_site_selected(*ds)) continue; - res.push_back(ds); + res.push_back(ds.get()); } #else /* walk the dive site list */ - int i; - const struct dive_site *ds; - for_each_dive_site (i, ds, divelog.sites) - res.push_back(get_dive_site(i, divelog.sites)); + for (const auto &ds: *divelog.sites) + res.push_back(ds.get()); #endif return res; } diff --git a/commands/command.cpp b/commands/command.cpp index 589bb9599..5b4ae93a2 100644 --- a/commands/command.cpp +++ b/commands/command.cpp @@ -134,9 +134,9 @@ void addDiveSite(const QString &name) execute(new AddDiveSite(name)); } -void importDiveSites(struct dive_site_table *sites, const QString &source) +void importDiveSites(dive_site_table sites, const QString &source) { - execute(new ImportDiveSites(sites, source)); + execute(new ImportDiveSites(std::move(sites), source)); } void mergeDiveSites(dive_site *ds, const QVector &sites) diff --git a/commands/command.h b/commands/command.h index e633d3a18..64e969942 100644 --- a/commands/command.h +++ b/commands/command.h @@ -68,7 +68,7 @@ void editDiveSiteCountry(dive_site *ds, const QString &value); void editDiveSiteLocation(dive_site *ds, location_t value); void editDiveSiteTaxonomy(dive_site *ds, taxonomy_data &value); // value is consumed (i.e. will be erased after call)! void addDiveSite(const QString &name); -void importDiveSites(struct dive_site_table *sites, const QString &source); +void importDiveSites(dive_site_table sites, const QString &source); // takes ownership of dive site table void mergeDiveSites(dive_site *ds, const QVector &sites); void purgeUnusedDiveSites(); diff --git a/commands/command_divelist.cpp b/commands/command_divelist.cpp index 22a8706e9..b7081f27f 100644 --- a/commands/command_divelist.cpp +++ b/commands/command_divelist.cpp @@ -142,9 +142,9 @@ DivesAndTripsToAdd DiveListBase::removeDives(DivesAndSitesToRemove &divesAndSite divesAndSitesToDelete.dives.clear(); for (dive_site *ds: divesAndSitesToDelete.sites) { - int idx = unregister_dive_site(ds); - sitesToAdd.emplace_back(ds); - emit diveListNotifier.diveSiteDeleted(ds, idx); + auto res = divelog.sites->pull(ds); + sitesToAdd.push_back(std::move(res.ptr)); + emit diveListNotifier.diveSiteDeleted(ds, res.idx); } divesAndSitesToDelete.sites.clear(); @@ -217,9 +217,9 @@ DivesAndSitesToRemove DiveListBase::addDives(DivesAndTripsToAdd &toAdd) // Finally, add any necessary dive sites for (std::unique_ptr &ds: toAdd.sites) { - sites.push_back(ds.get()); - int idx = register_dive_site(ds.release()); // Return ownership to backend - emit diveListNotifier.diveSiteAdded(sites.back(), idx); + auto res = divelog.sites->register_site(std::move(ds)); + sites.push_back(res.ptr); + emit diveListNotifier.diveSiteAdded(sites.back(), res.idx); } toAdd.sites.clear(); @@ -478,10 +478,10 @@ ImportDives::ImportDives(struct divelog *log, int flags, const QString &source) struct dive_table dives_to_add = empty_dive_table; struct dive_table dives_to_remove = empty_dive_table; struct trip_table trips_to_add = empty_trip_table; - struct dive_site_table sites_to_add = empty_dive_site_table; + dive_site_table sites_to_add; process_imported_dives(log, flags, &dives_to_add, &dives_to_remove, &trips_to_add, - &sites_to_add, &devicesToAddAndRemove); + sites_to_add, &devicesToAddAndRemove); // Add trips to the divesToAdd.trips structure divesToAdd.trips.reserve(trips_to_add.nr); @@ -489,9 +489,7 @@ ImportDives::ImportDives(struct divelog *log, int flags, const QString &source) divesToAdd.trips.emplace_back(trips_to_add.trips[i]); // Add sites to the divesToAdd.sites structure - divesToAdd.sites.reserve(sites_to_add.nr); - for (int i = 0; i < sites_to_add.nr; ++i) - divesToAdd.sites.emplace_back(sites_to_add.dive_sites[i]); + divesToAdd.sites = std::move(sites_to_add); // Add dives to the divesToAdd.dives structure divesToAdd.dives.reserve(dives_to_add.nr); @@ -525,7 +523,6 @@ ImportDives::ImportDives(struct divelog *log, int flags, const QString &source) free(dives_to_add.dives); free(dives_to_remove.dives); free(trips_to_add.trips); - free(sites_to_add.dive_sites); } bool ImportDives::workToBeDone() diff --git a/commands/command_divesite.cpp b/commands/command_divesite.cpp index 45eb29e3a..fda4572e6 100644 --- a/commands/command_divesite.cpp +++ b/commands/command_divesite.cpp @@ -30,9 +30,9 @@ static std::vector addDiveSites(std::vectorput(std::move(ds)); // Return ownership to backend. + res.push_back(add_res.ptr); + emit diveListNotifier.diveSiteAdded(res.back(), add_res.idx); // Inform frontend of new dive site. } emit diveListNotifier.divesChanged(changedDives, DiveField::DIVESITE); @@ -60,9 +60,9 @@ static std::vector> removeDiveSites(std::vectorpull(ds); + res.push_back(std::move(pull_res.ptr)); + emit diveListNotifier.diveSiteDeleted(ds, pull_res.idx); // Inform frontend of removed dive site. } emit diveListNotifier.divesChanged(changedDives, DiveField::DIVESITE); @@ -94,25 +94,18 @@ void AddDiveSite::undo() sitesToAdd = removeDiveSites(sitesToRemove); } -ImportDiveSites::ImportDiveSites(struct dive_site_table *sites, const QString &source) +ImportDiveSites::ImportDiveSites(dive_site_table sites, const QString &source) { setText(Command::Base::tr("import dive sites from %1").arg(source)); - for (int i = 0; i < sites->nr; ++i) { - struct dive_site *new_ds = sites->dive_sites[i]; - + for (auto &new_ds: sites) { // Don't import dive sites that already exist. Currently we only check for // the same name. We might want to be smarter here and merge dive site data, etc. - struct dive_site *old_ds = get_same_dive_site(new_ds); - if (old_ds) { - delete new_ds; + struct dive_site *old_ds = get_same_dive_site(*new_ds); + if (old_ds) continue; - } - sitesToAdd.emplace_back(new_ds); + sitesToAdd.push_back(std::move(new_ds)); } - - // All site have been consumed - sites->nr = 0; } bool ImportDiveSites::workToBeDone() @@ -153,10 +146,9 @@ void DeleteDiveSites::undo() PurgeUnusedDiveSites::PurgeUnusedDiveSites() { setText(Command::Base::tr("purge unused dive sites")); - for (int i = 0; i < divelog.sites->nr; ++i) { - dive_site *ds = divelog.sites->dive_sites[i]; + for (const auto &ds: *divelog.sites) { if (ds->dives.empty()) - sitesToRemove.push_back(ds); + sitesToRemove.push_back(ds.get()); } } @@ -391,7 +383,7 @@ ApplyGPSFixes::ApplyGPSFixes(const std::vector &fixes) siteLocations.push_back({ ds, dl.location }); } } else { - ds = create_dive_site(dl.name.toStdString(), divelog.sites); + ds = create_dive_site(dl.name.toStdString(), *divelog.sites); ds->location = dl.location; add_dive_to_dive_site(dl.d, ds); dl.d->dive_site = nullptr; // This will be set on redo() diff --git a/commands/command_divesite.h b/commands/command_divesite.h index 655f75a62..86ca1568d 100644 --- a/commands/command_divesite.h +++ b/commands/command_divesite.h @@ -36,8 +36,8 @@ private: class ImportDiveSites : public Base { public: - // Note: the dive site table is consumed after the call it will be empty. - ImportDiveSites(struct dive_site_table *sites, const QString &source); + // Note: Takes ownership of dive site table + ImportDiveSites(dive_site_table sites, const QString &source); private: bool workToBeDone() override; void undo() override; diff --git a/commands/command_edit.cpp b/commands/command_edit.cpp index 9b01d1ddd..b5735b844 100644 --- a/commands/command_edit.cpp +++ b/commands/command_edit.cpp @@ -401,17 +401,17 @@ EditDiveSiteNew::EditDiveSiteNew(const QString &newName, bool currentDiveOnly) : void EditDiveSiteNew::undo() { EditDiveSite::undo(); - int idx = unregister_dive_site(diveSiteToRemove); - diveSiteToAdd.reset(diveSiteToRemove); - emit diveListNotifier.diveSiteDeleted(diveSiteToRemove, idx); // Inform frontend of removed dive site. + auto res = divelog.sites->pull(diveSiteToRemove); + diveSiteToAdd = std::move(res.ptr); + emit diveListNotifier.diveSiteDeleted(diveSiteToRemove, res.idx); // Inform frontend of removed dive site. diveSiteToRemove = nullptr; } void EditDiveSiteNew::redo() { - diveSiteToRemove = diveSiteToAdd.get(); - int idx = register_dive_site(diveSiteToAdd.release()); // Return ownership to backend. - emit diveListNotifier.diveSiteAdded(diveSiteToRemove, idx); // Inform frontend of new dive site. + auto res = divelog.sites->register_site(std::move(diveSiteToAdd)); // Return ownership to backend. + diveSiteToRemove = res.ptr; + emit diveListNotifier.diveSiteAdded(diveSiteToRemove, res.idx); // Inform frontend of new dive site. EditDiveSite::redo(); } @@ -1439,9 +1439,9 @@ EditDive::EditDive(dive *oldDiveIn, dive *newDiveIn, dive_site *createDs, dive_s void EditDive::undo() { if (siteToRemove) { - int idx = unregister_dive_site(siteToRemove); - siteToAdd.reset(siteToRemove); - emit diveListNotifier.diveSiteDeleted(siteToRemove, idx); // Inform frontend of removed dive site. + auto res = divelog.sites->pull(siteToRemove); + siteToAdd = std::move(res.ptr); + emit diveListNotifier.diveSiteDeleted(siteToRemove, res.idx); // Inform frontend of removed dive site. } exchangeDives(); @@ -1451,9 +1451,9 @@ void EditDive::undo() void EditDive::redo() { if (siteToAdd) { - siteToRemove = siteToAdd.get(); - int idx = register_dive_site(siteToAdd.release()); // Return ownership to backend. - emit diveListNotifier.diveSiteAdded(siteToRemove, idx); // Inform frontend of new dive site. + auto res = divelog.sites->register_site(std::move(siteToAdd)); // Return ownership to backend. + siteToRemove = res.ptr; + emit diveListNotifier.diveSiteAdded(siteToRemove, res.idx); // Inform frontend of new dive site. } exchangeDives(); diff --git a/commands/command_pictures.cpp b/commands/command_pictures.cpp index 6c5ef8ea1..7777803ad 100644 --- a/commands/command_pictures.cpp +++ b/commands/command_pictures.cpp @@ -217,9 +217,9 @@ void AddPictures::undo() // Remove dive sites for (dive_site *siteToRemove: sitesToRemove) { - int idx = unregister_dive_site(siteToRemove); - sitesToAdd.emplace_back(siteToRemove); - emit diveListNotifier.diveSiteDeleted(siteToRemove, idx); // Inform frontend of removed dive site. + auto res = divelog.sites->pull(siteToRemove); + sitesToAdd.push_back(std::move(res.ptr)); + emit diveListNotifier.diveSiteDeleted(siteToRemove, res.idx); // Inform frontend of removed dive site. } sitesToRemove.clear(); } @@ -228,9 +228,9 @@ void AddPictures::redo() { // Add dive sites for (std::unique_ptr &siteToAdd: sitesToAdd) { - sitesToRemove.push_back(siteToAdd.get()); - int idx = register_dive_site(siteToAdd.release()); // Return ownership to backend. - emit diveListNotifier.diveSiteAdded(sitesToRemove.back(), idx); // Inform frontend of new dive site. + auto res = divelog.sites->register_site(std::move(siteToAdd)); // Return ownership to backend. + sitesToRemove.push_back(res.ptr); + emit diveListNotifier.diveSiteAdded(sitesToRemove.back(), res.idx); // Inform frontend of new dive site. } sitesToAdd.clear(); diff --git a/core/datatrak.cpp b/core/datatrak.cpp index c1f005756..95224e0b7 100644 --- a/core/datatrak.cpp +++ b/core/datatrak.cpp @@ -211,9 +211,9 @@ static char *dt_dive_parser(unsigned char *runner, struct dive *dt_dive, struct */ { std::string buffer2 = std::string((char *)locality) + " " + (char *)dive_point; - struct dive_site *ds = get_dive_site_by_name(buffer2, log->sites); + struct dive_site *ds = get_dive_site_by_name(buffer2, *log->sites); if (!ds) - ds = create_dive_site(buffer2, log->sites); + ds = create_dive_site(buffer2, *log->sites); add_dive_to_dive_site(dt_dive, ds); } free(locality); diff --git a/core/dive.h b/core/dive.h index 492e67fcf..8ffdf60f4 100644 --- a/core/dive.h +++ b/core/dive.h @@ -19,7 +19,6 @@ extern const char *divemode_text_ui[]; extern const char *divemode_text[]; struct dive_site; -struct dive_site_table; struct dive_table; struct dive_trip; struct full_text_cache; diff --git a/core/divelist.cpp b/core/divelist.cpp index 805345941..3be6fd3ec 100644 --- a/core/divelist.cpp +++ b/core/divelist.cpp @@ -950,13 +950,13 @@ void add_imported_dives(struct divelog *import_log, int flags) struct dive_table dives_to_add = empty_dive_table; struct dive_table dives_to_remove = empty_dive_table; struct trip_table trips_to_add = empty_trip_table; - struct dive_site_table dive_sites_to_add = empty_dive_site_table; + dive_site_table dive_sites_to_add; struct device_table *devices_to_add = alloc_device_table(); /* Process imported dives and generate lists of dives * to-be-added and to-be-removed */ process_imported_dives(import_log, flags, &dives_to_add, &dives_to_remove, &trips_to_add, - &dive_sites_to_add, devices_to_add); + dive_sites_to_add, devices_to_add); /* Start by deselecting all dives, so that we don't end up with an invalid selection */ select_single_dive(NULL); @@ -990,9 +990,8 @@ void add_imported_dives(struct divelog *import_log, int flags) trips_to_add.nr = 0; /* Add new dive sites */ - for (i = 0; i < dive_sites_to_add.nr; i++) - register_dive_site(dive_sites_to_add.dive_sites[i]); - dive_sites_to_add.nr = 0; + for (auto &ds: dive_sites_to_add) + divelog.sites->register_site(std::move(ds)); /* Add new devices */ for (i = 0; i < nr_devices(devices_to_add); i++) { @@ -1008,7 +1007,6 @@ void add_imported_dives(struct divelog *import_log, int flags) free(dives_to_add.dives); free(dives_to_remove.dives); free(trips_to_add.trips); - free(dive_sites_to_add.dive_sites); /* Inform frontend of reset data. This should reset all the models. */ emit_reset_signal(); @@ -1083,7 +1081,7 @@ bool try_to_merge_trip(struct dive_trip *trip_import, struct dive_table *import_ void process_imported_dives(struct divelog *import_log, int flags, /* output parameters: */ struct dive_table *dives_to_add, struct dive_table *dives_to_remove, - struct trip_table *trips_to_add, struct dive_site_table *sites_to_add, + struct trip_table *trips_to_add, dive_site_table &sites_to_add, struct device_table *devices_to_add) { int i, j, nr, start_renumbering_at = 0; @@ -1096,7 +1094,7 @@ void process_imported_dives(struct divelog *import_log, int flags, clear_dive_table(dives_to_add); clear_dive_table(dives_to_remove); clear_trip_table(trips_to_add); - clear_dive_site_table(sites_to_add); + sites_to_add.clear(); clear_device_table(devices_to_add); /* Check if any of the new dives has a number. This will be @@ -1130,36 +1128,33 @@ void process_imported_dives(struct divelog *import_log, int flags, autogroup_dives(import_log->dives, import_log->trips); /* If dive sites already exist, use the existing versions. */ - for (i = 0; i < import_log->sites->nr; i++) { - struct dive_site *new_ds = import_log->sites->dive_sites[i]; - struct dive_site *old_ds = get_same_dive_site(new_ds); + for (auto &new_ds: *import_log->sites) { + struct dive_site *old_ds = get_same_dive_site(*new_ds); /* Check if it dive site is actually used by new dives. */ for (j = 0; j < import_log->dives->nr; j++) { - if (import_log->dives->dives[j]->dive_site == new_ds) + if (import_log->dives->dives[j]->dive_site == new_ds.get()) break; } if (j == import_log->dives->nr) { - /* Dive site not even used - free it and go to next. */ - delete new_ds; + /* Dive site not even used. */ continue; } if (!old_ds) { /* Dive site doesn't exist. Add it to list of dive sites to be added. */ new_ds->dives.clear(); /* Caller is responsible for adding dives to site */ - add_dive_site_to_table(new_ds, sites_to_add); - continue; + sites_to_add.put(std::move(new_ds)); + } else { + /* Dive site already exists - use the old one. */ + for (j = 0; j < import_log->dives->nr; j++) { + if (import_log->dives->dives[j]->dive_site == new_ds.get()) + import_log->dives->dives[j]->dive_site = old_ds; + } } - /* Dive site already exists - use the old and free the new. */ - for (j = 0; j < import_log->dives->nr; j++) { - if (import_log->dives->dives[j]->dive_site == new_ds) - import_log->dives->dives[j]->dive_site = old_ds; - } - delete new_ds; } - import_log->sites->nr = 0; /* All dive sites were consumed */ + import_log->sites->clear(); /* Merge overlapping trips. Since both trip tables are sorted, we * could be smarter here, but realistically not a whole lot of trips diff --git a/core/divelist.h b/core/divelist.h index fed83f31c..3f8ceac07 100644 --- a/core/divelist.h +++ b/core/divelist.h @@ -7,7 +7,7 @@ struct dive; struct divelog; struct trip_table; -struct dive_site_table; +class dive_site_table; struct device_table; struct deco_state; @@ -34,7 +34,7 @@ extern void process_loaded_dives(); extern void add_imported_dives(struct divelog *log, int flags); extern void process_imported_dives(struct divelog *import_log, int flags, struct dive_table *dives_to_add, struct dive_table *dives_to_remove, - struct trip_table *trips_to_add, struct dive_site_table *sites_to_add, + struct trip_table *trips_to_add, dive_site_table &sites_to_add, struct device_table *devices_to_add); extern int dive_table_get_insertion_index(struct dive_table *table, struct dive *dive); diff --git a/core/divelog.cpp b/core/divelog.cpp index 5a4caea23..63be52081 100644 --- a/core/divelog.cpp +++ b/core/divelog.cpp @@ -22,14 +22,12 @@ divelog::divelog() : { *dives = empty_dive_table; *trips = empty_trip_table; - *sites = empty_dive_site_table; } divelog::~divelog() { clear_dive_table(dives); clear_trip_table(trips); - clear_dive_site_table(sites); delete dives; delete trips; delete sites; @@ -40,16 +38,14 @@ divelog::~divelog() divelog::divelog(divelog &&log) : dives(new dive_table), trips(new trip_table), - sites(new dive_site_table), + sites(new dive_site_table(std::move(*log.sites))), devices(new device_table), filter_presets(new filter_preset_table) { *dives = empty_dive_table; *trips = empty_trip_table; - *sites = empty_dive_site_table; move_dive_table(log.dives, dives); move_trip_table(log.trips, trips); - move_dive_site_table(log.sites, sites); *devices = std::move(*log.devices); *filter_presets = std::move(*log.filter_presets); } @@ -58,7 +54,7 @@ struct divelog &divelog::operator=(divelog &&log) { move_dive_table(log.dives, dives); move_trip_table(log.trips, trips); - move_dive_site_table(log.sites, sites); + *sites = std::move(*log.sites); *devices = std::move(*log.devices); *filter_presets = std::move(*log.filter_presets); return *this; @@ -82,8 +78,7 @@ void divelog::clear() { while (dives->nr > 0) delete_single_dive(this, dives->nr - 1); - while (sites->nr) - delete_dive_site(get_dive_site(0, sites), sites); + sites->clear(); if (trips->nr != 0) { report_info("Warning: trip table not empty in divelog::clear()!"); trips->nr = 0; diff --git a/core/divelog.h b/core/divelog.h index b3612a405..d153a863b 100644 --- a/core/divelog.h +++ b/core/divelog.h @@ -5,7 +5,7 @@ struct dive_table; struct trip_table; -struct dive_site_table; +class dive_site_table; struct device_table; struct filter_preset_table; @@ -14,7 +14,7 @@ struct filter_preset_table; struct divelog { struct dive_table *dives; struct trip_table *trips; - struct dive_site_table *sites; + dive_site_table *sites; struct device_table *devices; struct filter_preset_table *filter_presets; bool autogroup; diff --git a/core/divesite.cpp b/core/divesite.cpp index a767d6e5c..148c65026 100644 --- a/core/divesite.cpp +++ b/core/divesite.cpp @@ -10,71 +10,53 @@ #include "membuffer.h" #include "pref.h" #include "subsurface-string.h" -#include "table.h" #include "sha1.h" #include -int get_divesite_idx(const struct dive_site *ds, struct dive_site_table *ds_table) +int get_divesite_idx(const struct dive_site *ds, dive_site_table &ds_table) { - int i; - const struct dive_site *d; - // tempting as it may be, don't die when called with ds=NULL - if (ds) - for_each_dive_site(i, d, ds_table) { - if (d == ds) - return i; - } - return -1; + auto it = std::find_if(ds_table.begin(), ds_table.end(), [ds] (const auto &ds2) { return ds2.get() == ds; }); + return it != ds_table.end() ? it - ds_table.begin() : -1; } -// TODO: keep table sorted by UUID and do a binary search? -struct dive_site *get_dive_site_by_uuid(uint32_t uuid, struct dive_site_table *ds_table) +template +struct dive_site *get_dive_site_by_predicate(dive_site_table &ds_table, PRED pred) { - int i; - struct dive_site *ds; - for_each_dive_site (i, ds, ds_table) - if (ds->uuid == uuid) - return get_dive_site(i, ds_table); - return NULL; + auto it = std::find_if(ds_table.begin(), ds_table.end(), pred); + return it != ds_table.end() ? it->get() : NULL; +} + +struct dive_site *get_dive_site_by_uuid(uint32_t uuid, dive_site_table &ds_table) +{ + // The table is sorted by uuid + auto it = std::lower_bound(ds_table.begin(), ds_table.end(), uuid, + [] (const auto &ds, auto uuid) { return ds->uuid < uuid; }); + return it != ds_table.end() && (*it)->uuid == uuid ? it->get() : NULL; } /* there could be multiple sites of the same name - return the first one */ -struct dive_site *get_dive_site_by_name(const std::string &name, struct dive_site_table *ds_table) +struct dive_site *get_dive_site_by_name(const std::string &name, dive_site_table &ds_table) { - int i; - struct dive_site *ds; - for_each_dive_site (i, ds, ds_table) { - if (ds->name == name) - return ds; - } - return NULL; + return get_dive_site_by_predicate(ds_table, + [&name](const auto &ds) { return ds->name == name; }); } /* there could be multiple sites at the same GPS fix - return the first one */ -struct dive_site *get_dive_site_by_gps(const location_t *loc, struct dive_site_table *ds_table) +struct dive_site *get_dive_site_by_gps(const location_t *loc, dive_site_table &ds_table) { - int i; - struct dive_site *ds; - for_each_dive_site (i, ds, ds_table) { - if (*loc == ds->location) - return ds; - } - return NULL; + return get_dive_site_by_predicate(ds_table, + [loc](const auto &ds) { return ds->location == *loc; }); } /* to avoid a bug where we have two dive sites with different name and the same GPS coordinates * and first get the gps coordinates (reading a V2 file) and happen to get back "the other" name, * this function allows us to verify if a very specific name/GPS combination already exists */ -struct dive_site *get_dive_site_by_gps_and_name(const std::string &name, const location_t *loc, struct dive_site_table *ds_table) +struct dive_site *get_dive_site_by_gps_and_name(const std::string &name, const location_t *loc, dive_site_table &ds_table) { - int i; - struct dive_site *ds; - for_each_dive_site (i, ds, ds_table) { - if (*loc == ds->location && ds->name == name) - return ds; - } - return NULL; + return get_dive_site_by_predicate(ds_table, + [&name, loc](const auto &ds) { return ds->location == *loc && + ds->name == name; }); } // Calculate the distance in meters between two coordinates. @@ -96,52 +78,21 @@ unsigned int get_distance(const location_t *loc1, const location_t *loc2) } /* find the closest one, no more than distance meters away - if more than one at same distance, pick the first */ -struct dive_site *get_dive_site_by_gps_proximity(const location_t *loc, int distance, struct dive_site_table *ds_table) +struct dive_site *get_dive_site_by_gps_proximity(const location_t *loc, int distance, dive_site_table &ds_table) { - int i; - struct dive_site *ds, *res = NULL; + struct dive_site *res = nullptr; unsigned int cur_distance, min_distance = distance; - for_each_dive_site (i, ds, ds_table) { - if (dive_site_has_gps_location(ds) && + for (const auto &ds: ds_table) { + if (dive_site_has_gps_location(ds.get()) && (cur_distance = get_distance(&ds->location, loc)) < min_distance) { min_distance = cur_distance; - res = ds; + res = ds.get(); } } return res; } -int register_dive_site(struct dive_site *ds) -{ - return add_dive_site_to_table(ds, divelog.sites); -} - -static int compare_sites(const struct dive_site *a, const struct dive_site *b) -{ - return a->uuid > b->uuid ? 1 : a->uuid == b->uuid ? 0 : -1; -} - -static int site_less_than(const struct dive_site *a, const struct dive_site *b) -{ - return compare_sites(a, b) < 0; -} - -static void free_dive_site(struct dive_site *ds) -{ - delete ds; -} - -static MAKE_GROW_TABLE(dive_site_table, struct dive_site *, dive_sites) -static MAKE_GET_INSERTION_INDEX(dive_site_table, struct dive_site *, dive_sites, site_less_than) -static MAKE_ADD_TO(dive_site_table, struct dive_site *, dive_sites) -static MAKE_REMOVE_FROM(dive_site_table, dive_sites) -static MAKE_GET_IDX(dive_site_table, struct dive_site *, dive_sites) -MAKE_SORT(dive_site_table, struct dive_site *, dive_sites, compare_sites) -static MAKE_REMOVE(dive_site_table, struct dive_site *, dive_site) -MAKE_CLEAR_TABLE(dive_site_table, dive_sites, dive_site) -MAKE_MOVE_TABLE(dive_site_table, dive_sites) - -int add_dive_site_to_table(struct dive_site *ds, struct dive_site_table *ds_table) +dive_site_table::put_result dive_site_table::register_site(std::unique_ptr ds) { /* If the site doesn't yet have an UUID, create a new one. * Make this deterministic for testing. */ @@ -158,12 +109,10 @@ int add_dive_site_to_table(struct dive_site *ds, struct dive_site_table *ds_tabl /* Take care to never have the same uuid twice. This could happen on * reimport of a log where the dive sites have diverged */ - while (ds->uuid == 0 || get_dive_site_by_uuid(ds->uuid, ds_table) != NULL) + while (ds->uuid == 0 || get_dive_site_by_uuid(ds->uuid, *this) != NULL) ++ds->uuid; - int idx = dive_site_table_get_insertion_index(ds_table, ds); - add_to_dive_site_table(ds_table, idx, ds); - return idx; + return put(std::move(ds)); } dive_site::dive_site() @@ -178,24 +127,23 @@ dive_site::dive_site(const std::string &name, const location_t *loc) : name(name { } +dive_site::dive_site(uint32_t uuid) : uuid(uuid) +{ +} + dive_site::~dive_site() { } /* when parsing, dive sites are identified by uuid */ -struct dive_site *alloc_or_get_dive_site(uint32_t uuid, struct dive_site_table *ds_table) +struct dive_site *alloc_or_get_dive_site(uint32_t uuid, dive_site_table &ds_table) { struct dive_site *ds; if (uuid && (ds = get_dive_site_by_uuid(uuid, ds_table)) != NULL) return ds; - ds = new dive_site; - ds->uuid = uuid; - - add_dive_site_to_table(ds, ds_table); - - return ds; + return ds_table.register_site(std::make_unique(uuid)).ptr; } size_t nr_of_dives_at_dive_site(const dive_site &ds) @@ -209,33 +157,16 @@ bool is_dive_site_selected(const struct dive_site &ds) [](dive *dive) { return dive->selected; }); } -int unregister_dive_site(struct dive_site *ds) -{ - return remove_dive_site(ds, divelog.sites); -} - -void delete_dive_site(struct dive_site *ds, struct dive_site_table *ds_table) -{ - if (!ds) - return; - remove_dive_site(ds, ds_table); - delete ds; -} - /* allocate a new site and add it to the table */ -struct dive_site *create_dive_site(const std::string &name, struct dive_site_table *ds_table) +struct dive_site *create_dive_site(const std::string &name, dive_site_table &ds_table) { - struct dive_site *ds = new dive_site(name); - add_dive_site_to_table(ds, ds_table); - return ds; + return ds_table.register_site(std::make_unique(name)).ptr; } /* same as before, but with GPS data */ -struct dive_site *create_dive_site_with_gps(const std::string &name, const location_t *loc, struct dive_site_table *ds_table) +struct dive_site *create_dive_site_with_gps(const std::string &name, const location_t *loc, dive_site_table &ds_table) { - struct dive_site *ds = new dive_site(name, loc); - add_dive_site_to_table(ds, ds_table); - return ds; + return ds_table.register_site(std::make_unique(name, loc)).ptr; } /* if all fields are empty, the dive site is pointless */ @@ -270,22 +201,18 @@ static void merge_string(std::string &a, const std::string &b) * Taxonomy is not compared, as no taxonomy is generated on * import. */ -static bool same_dive_site(const struct dive_site *a, const struct dive_site *b) +static bool same_dive_site(const struct dive_site &a, const struct dive_site &b) { - return a->name == b->name - && a->location == b->location - && a->description == b->description - && a->notes == b->notes; + return a.name == b.name + && a.location == b.location + && a.description == b.description + && a.notes == b.notes; } -struct dive_site *get_same_dive_site(const struct dive_site *site) +struct dive_site *get_same_dive_site(const struct dive_site &site) { - int i; - struct dive_site *ds; - for_each_dive_site (i, ds, divelog.sites) - if (same_dive_site(ds, site)) - return ds; - return NULL; + return get_dive_site_by_predicate(*divelog.sites, + [site](const auto &ds) { return same_dive_site(*ds, site); }); } void merge_dive_site(struct dive_site *a, struct dive_site *b) @@ -299,32 +226,27 @@ void merge_dive_site(struct dive_site *a, struct dive_site *b) a->taxonomy = std::move(b->taxonomy); } -struct dive_site *find_or_create_dive_site_with_name(const std::string &name, struct dive_site_table *ds_table) +struct dive_site *find_or_create_dive_site_with_name(const std::string &name, dive_site_table &ds_table) { - int i; - struct dive_site *ds; - for_each_dive_site(i,ds, ds_table) { - if (name == ds->name) - break; - } + struct dive_site *ds = get_dive_site_by_name(name, ds_table); if (ds) return ds; return create_dive_site(name, ds_table); } -void purge_empty_dive_sites(struct dive_site_table *ds_table) +void purge_empty_dive_sites(dive_site_table &ds_table) { - int i, j; - struct dive *d; - struct dive_site *ds; - - for (i = 0; i < ds_table->nr; i++) { - ds = get_dive_site(i, ds_table); - if (!dive_site_is_empty(ds)) + for (const auto &ds: ds_table) { + if (!dive_site_is_empty(ds.get())) continue; - for_each_dive(j, d) { - if (d->dive_site == ds) + while (!ds->dives.empty()) { + struct dive *d = ds->dives.back(); + if (d->dive_site != ds.get()) { + report_info("Warning: dive %d registered to wrong dive site in %s", d->number, __func__); + ds->dives.pop_back(); + } else { unregister_dive_from_dive_site(d); + } } } } diff --git a/core/divesite.h b/core/divesite.h index 45885ffe7..85ade02d2 100644 --- a/core/divesite.h +++ b/core/divesite.h @@ -2,9 +2,10 @@ #ifndef DIVESITE_H #define DIVESITE_H -#include "units.h" -#include "taxonomy.h" #include "divelist.h" +#include "owning_table.h" +#include "taxonomy.h" +#include "units.h" #include #include @@ -21,52 +22,39 @@ struct dive_site dive_site(); dive_site(const std::string &name); dive_site(const std::string &name, const location_t *loc); + dive_site(uint32_t uuid); ~dive_site(); }; -typedef struct dive_site_table { - int nr, allocated; - struct dive_site **dive_sites; -} dive_site_table_t; - -static const dive_site_table_t empty_dive_site_table = { 0, 0, (struct dive_site **)0 }; - -static inline struct dive_site *get_dive_site(int nr, struct dive_site_table *ds_table) +inline int divesite_comp_uuid(const dive_site &ds1, const dive_site &ds2) { - if (nr >= ds_table->nr || nr < 0) - return NULL; - return ds_table->dive_sites[nr]; + if (ds1.uuid == ds2.uuid) + return 0; + return ds1.uuid < ds2.uuid ? -1 : 1; } -/* iterate over each dive site */ -#define for_each_dive_site(_i, _x, _ds_table) \ - for ((_i) = 0; ((_x) = get_dive_site(_i, _ds_table)) != NULL; (_i)++) +class dive_site_table : public sorted_owning_table { +public: + put_result register_site(std::unique_ptr site); +}; -int get_divesite_idx(const struct dive_site *ds, struct dive_site_table *ds_table); -struct dive_site *get_dive_site_by_uuid(uint32_t uuid, struct dive_site_table *ds_table); -void sort_dive_site_table(struct dive_site_table *ds_table); -int add_dive_site_to_table(struct dive_site *ds, struct dive_site_table *ds_table); -struct dive_site *alloc_or_get_dive_site(uint32_t uuid, struct dive_site_table *ds_table); -struct dive_site *alloc_dive_site(); +int get_divesite_idx(const struct dive_site *ds, dive_site_table &ds_table); +struct dive_site *get_dive_site_by_uuid(uint32_t uuid, dive_site_table &ds_table); +struct dive_site *alloc_or_get_dive_site(uint32_t uuid, dive_site_table &ds_table); size_t nr_of_dives_at_dive_site(const struct dive_site &ds); bool is_dive_site_selected(const struct dive_site &ds); -int unregister_dive_site(struct dive_site *ds); -int register_dive_site(struct dive_site *ds); -void delete_dive_site(struct dive_site *ds, struct dive_site_table *ds_table); -struct dive_site *create_dive_site(const std::string &name, struct dive_site_table *ds_table); -struct dive_site *create_dive_site_with_gps(const std::string &name, const location_t *, struct dive_site_table *ds_table); -struct dive_site *get_dive_site_by_name(const std::string &name, struct dive_site_table *ds_table); -struct dive_site *get_dive_site_by_gps(const location_t *, struct dive_site_table *ds_table); -struct dive_site *get_dive_site_by_gps_and_name(const std::string &name, const location_t *, struct dive_site_table *ds_table); -struct dive_site *get_dive_site_by_gps_proximity(const location_t *, int distance, struct dive_site_table *ds_table); -struct dive_site *get_same_dive_site(const struct dive_site *); +struct dive_site *create_dive_site(const std::string &name, dive_site_table &ds_table); +struct dive_site *create_dive_site_with_gps(const std::string &name, const location_t *, dive_site_table &ds_table); +struct dive_site *get_dive_site_by_name(const std::string &name, dive_site_table &ds_table); +struct dive_site *get_dive_site_by_gps(const location_t *, dive_site_table &ds_table); +struct dive_site *get_dive_site_by_gps_and_name(const std::string &name, const location_t *, dive_site_table &ds_table); +struct dive_site *get_dive_site_by_gps_proximity(const location_t *, int distance, dive_site_table &ds_table); +struct dive_site *get_same_dive_site(const struct dive_site &); bool dive_site_is_empty(struct dive_site *ds); void merge_dive_site(struct dive_site *a, struct dive_site *b); unsigned int get_distance(const location_t *loc1, const location_t *loc2); -struct dive_site *find_or_create_dive_site_with_name(const std::string &name, struct dive_site_table *ds_table); -void purge_empty_dive_sites(struct dive_site_table *ds_table); -void clear_dive_site_table(struct dive_site_table *ds_table); -void move_dive_site_table(struct dive_site_table *src, struct dive_site_table *dst); +struct dive_site *find_or_create_dive_site_with_name(const std::string &name, dive_site_table &ds_table); +void purge_empty_dive_sites(dive_site_table &ds_table); void add_dive_to_dive_site(struct dive *d, struct dive_site *ds); struct dive_site *unregister_dive_from_dive_site(struct dive *d); std::string constructLocationTags(const taxonomy_data &taxonomy, bool for_maintab); diff --git a/core/import-cobalt.cpp b/core/import-cobalt.cpp index ba30e63c5..43ad4b4c8 100644 --- a/core/import-cobalt.cpp +++ b/core/import-cobalt.cpp @@ -181,7 +181,7 @@ static int cobalt_dive(void *param, int, char **data, char **) if (location && location_site) { std::string tmp = std::string(location) + " / " + location_site; - add_dive_to_dive_site(state->cur_dive, find_or_create_dive_site_with_name(tmp, state->log->sites)); + add_dive_to_dive_site(state->cur_dive, find_or_create_dive_site_with_name(tmp, *state->log->sites)); } free(location); free(location_site); diff --git a/core/import-divinglog.cpp b/core/import-divinglog.cpp index 182a5ee88..1c020bf53 100644 --- a/core/import-divinglog.cpp +++ b/core/import-divinglog.cpp @@ -276,7 +276,7 @@ static int divinglog_dive(void *param, int, char **data, char **) state->cur_dive->when = (time_t)(atol(data[1])); if (data[2]) - add_dive_to_dive_site(state->cur_dive, find_or_create_dive_site_with_name(std::string(data[2]), state->log->sites)); + add_dive_to_dive_site(state->cur_dive, find_or_create_dive_site_with_name(std::string(data[2]), *state->log->sites)); if (data[3]) utf8_string(data[3], &state->cur_dive->buddy); diff --git a/core/libdivecomputer.cpp b/core/libdivecomputer.cpp index 4e77f2f8d..972adee91 100644 --- a/core/libdivecomputer.cpp +++ b/core/libdivecomputer.cpp @@ -636,7 +636,7 @@ static void parse_string_field(device_data_t *devdata, struct dive *dive, dc_fie if (location.lat.udeg && location.lon.udeg) { unregister_dive_from_dive_site(dive); - add_dive_to_dive_site(dive, create_dive_site_with_gps(std::string(str->value), &location, devdata->log->sites)); + add_dive_to_dive_site(dive, create_dive_site_with_gps(std::string(str->value), &location, *devdata->log->sites)); } } } diff --git a/core/liquivision.cpp b/core/liquivision.cpp index 660bd7df6..d098c573d 100644 --- a/core/liquivision.cpp +++ b/core/liquivision.cpp @@ -131,7 +131,7 @@ static int handle_event_ver3(int code, const unsigned char *ps, unsigned int ps_ return skip; } -static void parse_dives(int log_version, const unsigned char *buf, unsigned int buf_size, struct dive_table *table, struct dive_site_table *sites) +static void parse_dives(int log_version, const unsigned char *buf, unsigned int buf_size, struct dive_table *table, dive_site_table &sites) { unsigned int ptr = 0; unsigned char model; @@ -450,7 +450,7 @@ int try_to_open_liquivision(const char *, std::string &mem, struct divelog *log) } ptr += 4; - parse_dives(log_version, buf + ptr, buf_size - ptr, log->dives, log->sites); + parse_dives(log_version, buf + ptr, buf_size - ptr, log->dives, *log->sites); return 1; } diff --git a/core/load-git.cpp b/core/load-git.cpp index 43f8b7521..d9c61a698 100644 --- a/core/load-git.cpp +++ b/core/load-git.cpp @@ -175,9 +175,9 @@ static void parse_dive_gps(char *line, struct git_parser_state *state) parse_location(line, &location); if (!ds) { - ds = get_dive_site_by_gps(&location, state->log->sites); + ds = get_dive_site_by_gps(&location, *state->log->sites); if (!ds) - ds = create_dive_site_with_gps(std::string(), &location, state->log->sites); + ds = create_dive_site_with_gps(std::string(), &location, *state->log->sites); add_dive_to_dive_site(state->active_dive, ds); } else { if (dive_site_has_gps_location(ds) && ds->location != location) { @@ -221,9 +221,9 @@ static void parse_dive_location(char *, struct git_parser_state *state) std::string name = get_first_converted_string(state); struct dive_site *ds = get_dive_site_for_dive(state->active_dive); if (!ds) { - ds = get_dive_site_by_name(name, state->log->sites); + ds = get_dive_site_by_name(name, *state->log->sites); if (!ds) - ds = create_dive_site(name, state->log->sites); + ds = create_dive_site(name, *state->log->sites); add_dive_to_dive_site(state->active_dive, ds); } else { // we already had a dive site linked to the dive @@ -252,7 +252,7 @@ static void parse_dive_notes(char *, struct git_parser_state *state) { state->active_dive->notes = get_first_converted_string_c(state); } static void parse_dive_divesiteid(char *line, struct git_parser_state *state) -{ add_dive_to_dive_site(state->active_dive, get_dive_site_by_uuid(get_hex(line), state->log->sites)); } +{ add_dive_to_dive_site(state->active_dive, get_dive_site_by_uuid(get_hex(line), *state->log->sites)); } /* * We can have multiple tags. @@ -1711,7 +1711,7 @@ static int parse_site_entry(struct git_parser_state *state, const git_tree_entry if (*suffix == '\0') return report_error("Dive site without uuid"); uint32_t uuid = strtoul(suffix, NULL, 16); - state->active_site = alloc_or_get_dive_site(uuid, state->log->sites); + state->active_site = alloc_or_get_dive_site(uuid, *state->log->sites); git_blob *blob = git_tree_entry_blob(state->repo, entry); if (!blob) return report_error("Unable to read dive site file"); diff --git a/core/owning_table.h b/core/owning_table.h new file mode 100644 index 000000000..d70287469 --- /dev/null +++ b/core/owning_table.h @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Traditionally, most of our data structures are collected as tables + * (=arrays) of pointers. The advantage is that pointers (or references) + * to objects are stable, even if the array grows and reallocates. + * Implicitly, the table is the owner of the objects and the objets + * are deleted (freed) when removed from the table. + * This header defines an std::vector<> of unique_ptr<>s to make this + * explicit. + * + * The state I want to reach across the code base: whenever a part of the + * code owns a heap allocated object, it *always* possesses a unique_ptr<> + * to that object. All naked pointers are invariably considered as + * "non owning". + * + * There are two ways to end ownership: + * 1) The std::unique_ptr<> goes out of scope and the object is + * automatically deleted. + * 2) Ownership is passed to another std::unique_ptr<> using std::move(). + * + * This means that when adding an object to an owning_table, + * ownership of a std::unique_ptr<> is given up with std::move(). + * The table then returns a non-owning pointer to the object and + * optionally the insertion index. + * + * In converse, when removing an object, one provides a non-owning + * pointer, which is turned into an owning std::unique_ptr<> and + * the index where the object was removed. + * When ignoring the returned owning pointer, the object is + * automatically freed. + * + * The functions to add an entry to the table are called "put()", + * potentially with a suffix. The functions to remove an entry + * are called "pull()", likewise with an optional suffix. + * + * There are two versions of the table: + * 1) An unordered version, where the caller is responsible for + * adding at specified positions (either given by an index or at the end). + * Removal via a non-owning pointer is implemented by a linear search + * over the whole table. + * 2) An ordered version, where a comparison function that returns -1, 0, 1 + * is used to add objects. In that case, the caller must make sure that + * no two ojects that compare equal are added to the table. + * Obviously, the compare function is supposed to be replaced by the + * "spaceship operator" in due course. + * Here, adding and removing via non-owning pointers is implemented + * using a binary search. + * + * Note that, since the table contains std::unique_ptr<>s, to loop over + * all entries, it is best to use something such as + * for (const auto &ptr: table) ... + * I plan to add iterator adapters, that autometically dereference + * the unique_ptr<>s and provide const-references for const-tables. + * + * Time will tell how useful this class is. + */ +#ifndef CORE_OWNING_TABLE_H +#define CORE_OWNING_TABLE_H + +#include "errorhelper.h" + +#include +#include +#include +#include + +template +class owning_table : public std::vector> { +public: + struct put_result { + T *ptr; + size_t idx; + }; + struct pull_result { + std::unique_ptr ptr; + size_t idx; + }; + size_t get_idx(const T *item) const { + auto it = std::find_if(this->begin(), this->end(), + [item] (auto &i1) {return i1.get() == item; }); + return it != this->end() ? it - this->begin() : std::string::npos; + } + T *put_at(std::unique_ptr item, size_t idx) { + T *res = item.get(); + insert(this->begin() + idx, std::move(item)); + return res; + } + // Returns index of added item + put_result put_back(std::unique_ptr item) { + T *ptr = item.get(); + push_back(std::move(item)); + return { ptr, this->size() - 1 }; + } + std::unique_ptr pull_at(size_t idx) { + auto it = this->begin() + idx; + std::unique_ptr res = std::move(*it); + this->erase(it); + return res; + } + pull_result pull(const T *item) { + size_t idx = get_idx(item); + if (idx == std::string::npos) { + report_info("Warning: removing unexisting item in %s", __func__); + return { std::unique_ptr(), std::string::npos }; + } + return { pull_at(idx), idx }; + } +}; + +// Note: there must not be any elements that compare equal! +template +class sorted_owning_table : public owning_table { +public: + using typename owning_table::put_result; + using typename owning_table::pull_result; + // Returns index of added item + put_result put(std::unique_ptr item) { + auto it = std::lower_bound(this->begin(), this->end(), item, + [] (const auto &i1, const auto &i2) + { return CMP(*i1, *i2) < 0; }); + if (it != this->end() && CMP(**it, *item) == 0) + report_info("Warning: adding duplicate item in %s", __func__); + size_t idx = it - this->begin(); + T *ptr = item.get(); + this->insert(it, std::move(item)); + return { ptr, idx }; + } + // Optimized version of get_idx(), which uses binary search + // If not found, fall back to linear search and emit a warning. + // Note: this is probaly slower than a linesr search. But for now, + // it helps finding consistency problems. + size_t get_idx(const T *item) const { + auto it = std::lower_bound(this->begin(), this->end(), item, + [] (const auto &i1, const auto &i2) + { return CMP(*i1, *i2) < 0; }); + if (it == this->end() || CMP(**it, *item) != 0) { + size_t idx = owning_table::get_idx(item); + if (idx != std::string::npos) + report_info("Warning: index found by linear but not by binary search in %s", __func__); + return idx; + } + return it - this->begin(); + } + pull_result pull(const T *item) { + size_t idx = get_idx(item); + if (idx == std::string::npos) { + report_info("Warning: removing unexisting item in %s", __func__); + return { std::unique_ptr(), std::string::npos }; + } + return { this->pull_at(idx), idx }; + } +}; + +#endif diff --git a/core/parse-xml.cpp b/core/parse-xml.cpp index bc826fc3a..a4f153f93 100644 --- a/core/parse-xml.cpp +++ b/core/parse-xml.cpp @@ -559,7 +559,7 @@ static void dive_site(const char *buffer, struct dive *d, struct parser_state *s { uint32_t uuid; hex_value(buffer, &uuid); - add_dive_to_dive_site(d, get_dive_site_by_uuid(uuid, state->log->sites)); + add_dive_to_dive_site(d, get_dive_site_by_uuid(uuid, *state->log->sites)); } static void get_notrip(const char *buffer, bool *notrip) @@ -977,9 +977,9 @@ static void divinglog_place(const char *place, struct dive *d, struct parser_sta !state->city.empty() ? state->city.c_str() : "", !state->country.empty() ? ", " : "", !state->country.empty() ? state->country.c_str() : ""); - ds = get_dive_site_by_name(buffer, state->log->sites); + ds = get_dive_site_by_name(buffer, *state->log->sites); if (!ds) - ds = create_dive_site(buffer, state->log->sites); + ds = create_dive_site(buffer, *state->log->sites); add_dive_to_dive_site(d, ds); // TODO: capture the country / city info in the taxonomy instead @@ -1149,7 +1149,7 @@ static void gps_lat(const char *buffer, struct dive *dive, struct parser_state * location.lat = parse_degrees(buffer, &end); if (!ds) { - add_dive_to_dive_site(dive, create_dive_site_with_gps(std::string(), &location, state->log->sites)); + add_dive_to_dive_site(dive, create_dive_site_with_gps(std::string(), &location, *state->log->sites)); } else { if (ds->location.lat.udeg && ds->location.lat.udeg != location.lat.udeg) report_info("Oops, changing the latitude of existing dive site id %8x name %s; not good", ds->uuid, @@ -1166,7 +1166,7 @@ static void gps_long(const char *buffer, struct dive *dive, struct parser_state location.lon = parse_degrees(buffer, &end); if (!ds) { - add_dive_to_dive_site(dive, create_dive_site_with_gps(std::string(), &location, state->log->sites)); + add_dive_to_dive_site(dive, create_dive_site_with_gps(std::string(), &location, *state->log->sites)); } else { if (ds->location.lon.udeg && ds->location.lon.udeg != location.lon.udeg) report_info("Oops, changing the longitude of existing dive site id %8x name %s; not good", ds->uuid, @@ -1197,14 +1197,14 @@ static void gps_in_dive(const char *buffer, struct dive *dive, struct parser_sta parse_location(buffer, &location); if (!ds) { // check if we have a dive site within 20 meters of that gps fix - ds = get_dive_site_by_gps_proximity(&location, 20, state->log->sites); + ds = get_dive_site_by_gps_proximity(&location, 20, *state->log->sites); if (ds) { // found a site nearby; in case it turns out this one had a different name let's // remember the original coordinates so we can create the correct dive site later state->cur_location = location; } else { - ds = create_dive_site_with_gps(std::string(), &location, state->log->sites); + ds = create_dive_site_with_gps(std::string(), &location, *state->log->sites); } add_dive_to_dive_site(dive, ds); } else { @@ -2225,7 +2225,7 @@ int parse_dlf_buffer(unsigned char *buffer, size_t size, struct divelog *log) /* Measure GPS */ state.cur_location.lat.udeg = (int)((ptr[7] << 24) + (ptr[6] << 16) + (ptr[5] << 8) + (ptr[4] << 0)); state.cur_location.lon.udeg = (int)((ptr[11] << 24) + (ptr[10] << 16) + (ptr[9] << 8) + (ptr[8] << 0)); - add_dive_to_dive_site(state.cur_dive, create_dive_site_with_gps("DLF imported"s, &state.cur_location, state.log->sites)); + add_dive_to_dive_site(state.cur_dive, create_dive_site_with_gps("DLF imported"s, &state.cur_location, *state.log->sites)); break; default: break; diff --git a/core/parse.cpp b/core/parse.cpp index 17b5e9952..80af477b2 100644 --- a/core/parse.cpp +++ b/core/parse.cpp @@ -199,7 +199,7 @@ void dive_site_end(struct parser_state *state) if (!state->cur_dive_site) return; - struct dive_site *ds = alloc_or_get_dive_site(state->cur_dive_site->uuid, state->log->sites); + struct dive_site *ds = alloc_or_get_dive_site(state->cur_dive_site->uuid, *state->log->sites); merge_dive_site(ds, state->cur_dive_site.get()); if (verbose > 3) @@ -470,7 +470,7 @@ void add_dive_site(const char *ds_name, struct dive *dive, struct parser_state * struct dive_site *ds = dive->dive_site; if (!ds) { // if the dive doesn't have a dive site, check if there's already a dive site by this name - ds = get_dive_site_by_name(trimmed, state->log->sites); + ds = get_dive_site_by_name(trimmed, *state->log->sites); } if (ds) { // we have a dive site, let's hope there isn't a different name @@ -481,12 +481,12 @@ void add_dive_site(const char *ds_name, struct dive *dive, struct parser_state * // but wait, we could have gotten this one based on GPS coords and could // have had two different names for the same site... so let's search the other // way around - struct dive_site *exact_match = get_dive_site_by_gps_and_name(trimmed, &ds->location, state->log->sites); + struct dive_site *exact_match = get_dive_site_by_gps_and_name(trimmed, &ds->location, *state->log->sites); if (exact_match) { unregister_dive_from_dive_site(dive); add_dive_to_dive_site(dive, exact_match); } else { - struct dive_site *newds = create_dive_site(trimmed.c_str(), state->log->sites); + struct dive_site *newds = create_dive_site(trimmed.c_str(), *state->log->sites); unregister_dive_from_dive_site(dive); add_dive_to_dive_site(dive, newds); if (has_location(&state->cur_location)) { @@ -504,7 +504,7 @@ void add_dive_site(const char *ds_name, struct dive *dive, struct parser_state * add_dive_to_dive_site(dive, ds); } } else { - add_dive_to_dive_site(dive, create_dive_site(trimmed, state->log->sites)); + add_dive_to_dive_site(dive, create_dive_site(trimmed, *state->log->sites)); } } } diff --git a/core/save-git.cpp b/core/save-git.cpp index 87138351a..a74ca2f45 100644 --- a/core/save-git.cpp +++ b/core/save-git.cpp @@ -922,10 +922,9 @@ static void save_divesites(git_repository *repo, struct dir *tree) put_format(&dirname, "01-Divesites"); subdir = new_directory(repo, tree, &dirname); - purge_empty_dive_sites(divelog.sites); - for (int i = 0; i < divelog.sites->nr; i++) { + purge_empty_dive_sites(*divelog.sites); + for (const auto &ds: *divelog.sites) { membuffer b; - struct dive_site *ds = get_dive_site(i, divelog.sites); membuffer site_file_name; put_format(&site_file_name, "Site-%08x", ds->uuid); show_utf8(&b, "name ", ds->name.c_str(), "\n"); diff --git a/core/save-xml.cpp b/core/save-xml.cpp index 1affe29bd..44debcd46 100644 --- a/core/save-xml.cpp +++ b/core/save-xml.cpp @@ -701,10 +701,9 @@ static void save_dives_buffer(struct membuffer *b, bool select_only, bool anonym /* save the dive sites */ put_format(b, "\n"); - for (i = 0; i < divelog.sites->nr; i++) { - struct dive_site *ds = get_dive_site(i, divelog.sites); + for (const auto &ds: *divelog.sites) { /* Don't export empty dive sites */ - if (dive_site_is_empty(ds)) + if (dive_site_is_empty(ds.get())) continue; /* Only write used dive sites when exporting selected dives */ if (select_only && !is_dive_site_selected(*ds)) diff --git a/core/uemis-downloader.cpp b/core/uemis-downloader.cpp index 496f7e5e7..5fd76ef8e 100644 --- a/core/uemis-downloader.cpp +++ b/core/uemis-downloader.cpp @@ -915,7 +915,7 @@ static bool process_raw_buffer(device_data_t *devdata, uint32_t deviceid, std::s } else if (!is_log && dive && tag == "divespot_id") { int divespot_id; if (from_chars(val, divespot_id).ec != std::errc::invalid_argument) { - struct dive_site *ds = create_dive_site("from Uemis"s, devdata->log->sites); + struct dive_site *ds = create_dive_site("from Uemis"s, *devdata->log->sites); unregister_dive_from_dive_site(dive); add_dive_to_dive_site(dive, ds); uemis_obj.mark_divelocation(dive->dc.diveid, divespot_id, ds); @@ -1109,21 +1109,20 @@ static void get_uemis_divespot(device_data_t *devdata, const std::string &mountp * we search all existing divesites if we have one with the same name already. The function * returns the first found which is luckily not the newly created. */ - struct dive_site *ods = get_dive_site_by_name(nds->name, devdata->log->sites); - if (ods) { + struct dive_site *ods = get_dive_site_by_name(nds->name, *devdata->log->sites); + if (ods && nds->uuid != ods->uuid) { /* if the uuid's are the same, the new site is a duplicate and can be deleted */ - if (nds->uuid != ods->uuid) { - delete_dive_site(nds, devdata->log->sites); - unregister_dive_from_dive_site(dive); - add_dive_to_dive_site(dive, ods); - } + unregister_dive_from_dive_site(dive); + add_dive_to_dive_site(dive, ods); + devdata->log->sites->pull(nds); } divespot_mapping[divespot_id] = dive->dive_site; } else { /* if we can't load the dive site details, delete the site we * created in process_raw_buffer */ - delete_dive_site(dive->dive_site, devdata->log->sites); + devdata->log->sites->pull(dive->dive_site); + dive->dive_site = nullptr; } } } diff --git a/desktop-widgets/divesiteimportdialog.cpp b/desktop-widgets/divesiteimportdialog.cpp index 55dfaf7e1..237f3d5ea 100644 --- a/desktop-widgets/divesiteimportdialog.cpp +++ b/desktop-widgets/divesiteimportdialog.cpp @@ -10,20 +10,18 @@ #include -// Caller keeps ownership of "imported". The contents of "imported" will be consumed on execution of the dialog. -// On return, it will be empty. -DivesiteImportDialog::DivesiteImportDialog(struct dive_site_table &imported, QString source, QWidget *parent) : QDialog(parent), - importedSource(std::move(source)) +DivesiteImportDialog::DivesiteImportDialog(dive_site_table imported, QString source, QWidget *parent) : QDialog(parent), + importedSites(std::move(imported)), + importedSource(std::move(source)), + divesiteImportedModel(std::make_unique()) { QShortcut *close = new QShortcut(QKeySequence(Qt::CTRL | Qt::Key_W), this); QShortcut *quit = new QShortcut(QKeySequence(Qt::CTRL | Qt::Key_Q), this); - divesiteImportedModel = new DivesiteImportedModel(this); - int startingWidth = defaultModelFont().pointSize(); ui.setupUi(this); - ui.importedDivesitesView->setModel(divesiteImportedModel); + ui.importedDivesitesView->setModel(divesiteImportedModel.get()); ui.importedDivesitesView->setSelectionBehavior(QAbstractItemView::SelectRows); ui.importedDivesitesView->setSelectionMode(QAbstractItemView::SingleSelection); ui.importedDivesitesView->horizontalHeader()->setStretchLastSection(true); @@ -35,45 +33,36 @@ DivesiteImportDialog::DivesiteImportDialog(struct dive_site_table &imported, QSt ui.selectAllButton->setEnabled(true); ui.unselectAllButton->setEnabled(true); - connect(ui.importedDivesitesView, &QTableView::clicked, divesiteImportedModel, &DivesiteImportedModel::changeSelected); - connect(ui.selectAllButton, &QPushButton::clicked, divesiteImportedModel, &DivesiteImportedModel::selectAll); - connect(ui.unselectAllButton, &QPushButton::clicked, divesiteImportedModel, &DivesiteImportedModel::selectNone); + connect(ui.importedDivesitesView, &QTableView::clicked, divesiteImportedModel.get(), &DivesiteImportedModel::changeSelected); + connect(ui.selectAllButton, &QPushButton::clicked, divesiteImportedModel.get(), &DivesiteImportedModel::selectAll); + connect(ui.unselectAllButton, &QPushButton::clicked, divesiteImportedModel.get(), &DivesiteImportedModel::selectNone); connect(close, SIGNAL(activated()), this, SLOT(close())); connect(quit, SIGNAL(activated()), parent, SLOT(close())); ui.ok->setEnabled(true); - importedSites = imported; - imported.nr = imported.allocated = 0; - imported.dive_sites = nullptr; - divesiteImportedModel->repopulate(&importedSites); } DivesiteImportDialog::~DivesiteImportDialog() { - clear_dive_site_table(&importedSites); } void DivesiteImportDialog::on_cancel_clicked() { - clear_dive_site_table(&importedSites); done(-1); } void DivesiteImportDialog::on_ok_clicked() { // delete non-selected dive sites - struct dive_site_table selectedSites = empty_dive_site_table; - for (int i = 0; i < importedSites.nr; i++) - if (divesiteImportedModel->data(divesiteImportedModel->index(i, 0), Qt::CheckStateRole) == Qt::Checked) { - struct dive_site *newSite = new dive_site; - *newSite = *importedSites.dive_sites[i]; - add_dive_site_to_table(newSite, &selectedSites); - } + dive_site_table selectedSites; + for (size_t i = 0; i < importedSites.size(); i++) { + if (divesiteImportedModel->data(divesiteImportedModel->index(i, 0), Qt::CheckStateRole) == Qt::Checked) + selectedSites.push_back(std::move(importedSites[i])); + } + importedSites.clear(); // Hopefully, the model is not used thereafter! - Command::importDiveSites(&selectedSites, importedSource); - clear_dive_site_table(&selectedSites); - clear_dive_site_table(&importedSites); + Command::importDiveSites(std::move(selectedSites), importedSource); accept(); } diff --git a/desktop-widgets/divesiteimportdialog.h b/desktop-widgets/divesiteimportdialog.h index b137234e0..5af296830 100644 --- a/desktop-widgets/divesiteimportdialog.h +++ b/desktop-widgets/divesiteimportdialog.h @@ -21,7 +21,8 @@ class DivesiteImportedModel; class DivesiteImportDialog : public QDialog { Q_OBJECT public: - DivesiteImportDialog(struct dive_site_table &imported, QString source, QWidget *parent = 0 ); + // Note: takes ownership of importedd table + DivesiteImportDialog(dive_site_table imported, QString source, QWidget *parent = 0); ~DivesiteImportDialog(); public @@ -31,10 +32,10 @@ slots: private: Ui::DivesiteImportDialog ui; - struct dive_site_table importedSites; + dive_site_table importedSites; QString importedSource; - DivesiteImportedModel *divesiteImportedModel; + std::unique_ptr divesiteImportedModel; }; #endif // DIVESITEIMPORTDIALOG_H diff --git a/desktop-widgets/divesitelistview.cpp b/desktop-widgets/divesitelistview.cpp index 48b3a9ab3..6ec66cdd3 100644 --- a/desktop-widgets/divesitelistview.cpp +++ b/desktop-widgets/divesitelistview.cpp @@ -97,7 +97,7 @@ void DiveSiteListView::diveSiteAdded(struct dive_site *, int idx) void DiveSiteListView::diveSiteChanged(struct dive_site *ds, int field) { - int idx = get_divesite_idx(ds, divelog.sites); + int idx = get_divesite_idx(ds, *divelog.sites); if (idx < 0) return; QModelIndex globalIdx = LocationInformationModel::instance()->index(idx, field); diff --git a/desktop-widgets/locationinformation.cpp b/desktop-widgets/locationinformation.cpp index 9c3a44791..a04cb4a8e 100644 --- a/desktop-widgets/locationinformation.cpp +++ b/desktop-widgets/locationinformation.cpp @@ -388,9 +388,9 @@ bool DiveLocationFilterProxyModel::lessThan(const QModelIndex &source_left, cons // If there is a current location, sort by that - otherwise use the provided column if (has_location(¤tLocation)) { // The dive sites are -2 because of the first two items. - struct dive_site *ds1 = get_dive_site(source_left.row() - 2, divelog.sites); - struct dive_site *ds2 = get_dive_site(source_right.row() - 2, divelog.sites); - return get_distance(&ds1->location, ¤tLocation) < get_distance(&ds2->location, ¤tLocation); + auto loc1 = (*divelog.sites)[source_left.row() - 2]->location; + auto loc2 = (*divelog.sites)[source_right.row() - 2]->location; + return get_distance(&loc1, ¤tLocation) < get_distance(&loc2, ¤tLocation); } return source_left.data().toString().compare(source_right.data().toString(), Qt::CaseInsensitive) < 0; } @@ -411,6 +411,9 @@ QVariant DiveLocationModel::data(const QModelIndex &index, int role) const static const QIcon plusIcon(":list-add-icon"); static const QIcon geoCode(":geotag-icon"); + if (index.row() < 0 || index.row() >= (int)divelog.sites->size() + 2) + return QVariant(); + if (index.row() <= 1) { // two special cases. if (index.column() == LocationInformationModel::DIVESITE) return QVariant::fromValue(RECENTLY_ADDED_DIVESITE); @@ -428,8 +431,8 @@ QVariant DiveLocationModel::data(const QModelIndex &index, int role) const } // The dive sites are -2 because of the first two items. - struct dive_site *ds = get_dive_site(index.row() - 2, divelog.sites); - return LocationInformationModel::getDiveSiteData(ds, index.column(), role); + const auto &ds = (*divelog.sites)[index.row() - 2]; + return LocationInformationModel::getDiveSiteData(*ds, index.column(), role); } int DiveLocationModel::columnCount(const QModelIndex&) const @@ -439,7 +442,7 @@ int DiveLocationModel::columnCount(const QModelIndex&) const int DiveLocationModel::rowCount(const QModelIndex&) const { - return divelog.sites->nr + 2; + return (int)divelog.sites->size() + 2; } Qt::ItemFlags DiveLocationModel::flags(const QModelIndex &index) const @@ -560,12 +563,10 @@ void DiveLocationLineEdit::refreshDiveSiteCache() static struct dive_site *get_dive_site_name_start_which_str(const QString &str) { - struct dive_site *ds; - int i; - for_each_dive_site (i, ds, divelog.sites) { + for (const auto &ds: *divelog.sites) { QString dsName = QString::fromStdString(ds->name); if (dsName.toLower().startsWith(str.toLower())) - return ds; + return ds.get(); } return NULL; } diff --git a/desktop-widgets/mainwindow.cpp b/desktop-widgets/mainwindow.cpp index 65a331584..c8bee4a8b 100644 --- a/desktop-widgets/mainwindow.cpp +++ b/desktop-widgets/mainwindow.cpp @@ -1409,12 +1409,12 @@ void MainWindow::on_actionImportDiveSites_triggered() parse_file(fileNamePtr.data(), &log); } // The imported dive sites still have pointers to imported dives - remove them - for (int i = 0; i < log.sites->nr; ++i) - log.sites->dive_sites[i]->dives.clear(); + for (const auto &ds: *log.sites) + ds->dives.clear(); QString source = fileNames.size() == 1 ? fileNames[0] : tr("multiple files"); - DivesiteImportDialog divesiteImport(*log.sites, source, this); + DivesiteImportDialog divesiteImport(std::move(*log.sites), source, this); divesiteImport.exec(); } diff --git a/mobile-widgets/qmlmanager.cpp b/mobile-widgets/qmlmanager.cpp index 44d07c607..694d53642 100644 --- a/mobile-widgets/qmlmanager.cpp +++ b/mobile-widgets/qmlmanager.cpp @@ -1069,7 +1069,7 @@ bool QMLManager::checkLocation(DiveSiteChange &res, struct dive *d, QString loca bool changed = false; QString oldLocation = QString::fromStdString(get_dive_location(d)); if (oldLocation != location) { - ds = get_dive_site_by_name(location.toStdString(), divelog.sites); + ds = get_dive_site_by_name(location.toStdString(), *divelog.sites); if (!ds && !location.isEmpty()) { res.createdDs = std::make_unique(qPrintable(location)); res.changed = true; @@ -1816,7 +1816,7 @@ QString QMLManager::getVersion() const QString QMLManager::getGpsFromSiteName(const QString &siteName) { - struct dive_site *ds = get_dive_site_by_name(siteName.toStdString(), divelog.sites); + struct dive_site *ds = get_dive_site_by_name(siteName.toStdString(), *divelog.sites); if (!ds) return QString(); return printGPSCoords(&ds->location); diff --git a/qt-models/divelocationmodel.cpp b/qt-models/divelocationmodel.cpp index 5fbf48f7d..1dd73ec6e 100644 --- a/qt-models/divelocationmodel.cpp +++ b/qt-models/divelocationmodel.cpp @@ -37,7 +37,7 @@ int LocationInformationModel::columnCount(const QModelIndex &) const int LocationInformationModel::rowCount(const QModelIndex &) const { - return divelog.sites->nr; + return (int)divelog.sites->size(); } QVariant LocationInformationModel::headerData(int section, Qt::Orientation orientation, int role) const @@ -81,21 +81,18 @@ Qt::ItemFlags LocationInformationModel::flags(const QModelIndex &index) const return QAbstractItemModel::flags(index); } -QVariant LocationInformationModel::getDiveSiteData(const struct dive_site *ds, int column, int role) +QVariant LocationInformationModel::getDiveSiteData(const struct dive_site &ds, int column, int role) { - if (!ds) - return QVariant(); - switch(role) { case Qt::EditRole: case Qt::DisplayRole: switch(column) { - case DIVESITE: return QVariant::fromValue((dive_site *)ds); // Not nice: casting away const - case NAME: return QString::fromStdString(ds->name); - case NUM_DIVES: return static_cast(ds->dives.size()); + case DIVESITE: return QVariant::fromValue((dive_site *)&ds); // Not nice: casting away const + case NAME: return QString::fromStdString(ds.name); + case NUM_DIVES: return static_cast(ds.dives.size()); case LOCATION: return "TODO"; - case DESCRIPTION: return QString::fromStdString(ds->description); - case NOTES: return QString::fromStdString(ds->notes); + case DESCRIPTION: return QString::fromStdString(ds.description); + case NOTES: return QString::fromStdString(ds.notes); case TAXONOMY: return "TODO"; } break; @@ -111,22 +108,22 @@ QVariant LocationInformationModel::getDiveSiteData(const struct dive_site *ds, i case EDIT: return editIcon(); case REMOVE: return trashIcon(); #endif - case NAME: return dive_site_has_gps_location(ds) ? QIcon(":geotag-icon") : QVariant(); + case NAME: return dive_site_has_gps_location(&ds) ? QIcon(":geotag-icon") : QVariant(); } break; case DIVESITE_ROLE: - return QVariant::fromValue((dive_site *)ds); // Not nice: casting away const + return QVariant::fromValue((dive_site *)&ds); // Not nice: casting away const } return QVariant(); } QVariant LocationInformationModel::data(const QModelIndex &index, int role) const { - if (!index.isValid()) + if (!index.isValid() || index.row() >= (int)divelog.sites->size()) return QVariant(); - struct dive_site *ds = get_dive_site(index.row(), divelog.sites); - return getDiveSiteData(ds, index.column(), role); + const auto &ds = (*divelog.sites)[index.row()].get(); + return getDiveSiteData(*ds, index.column(), role); } void LocationInformationModel::update() @@ -137,7 +134,7 @@ void LocationInformationModel::update() void LocationInformationModel::diveSiteDiveCountChanged(dive_site *ds) { - int idx = get_divesite_idx(ds, divelog.sites); + int idx = get_divesite_idx(ds, *divelog.sites); if (idx >= 0) dataChanged(createIndex(idx, NUM_DIVES), createIndex(idx, NUM_DIVES)); } @@ -162,7 +159,7 @@ void LocationInformationModel::diveSiteDeleted(struct dive_site *, int idx) void LocationInformationModel::diveSiteChanged(struct dive_site *ds, int field) { - int idx = get_divesite_idx(ds, divelog.sites); + int idx = get_divesite_idx(ds, *divelog.sites); if (idx < 0) return; dataChanged(createIndex(idx, field), createIndex(idx, field)); @@ -170,7 +167,7 @@ void LocationInformationModel::diveSiteChanged(struct dive_site *ds, int field) void LocationInformationModel::diveSiteDivesChanged(struct dive_site *ds) { - int idx = get_divesite_idx(ds, divelog.sites); + int idx = get_divesite_idx(ds, *divelog.sites); if (idx < 0) return; dataChanged(createIndex(idx, NUM_DIVES), createIndex(idx, NUM_DIVES)); @@ -181,9 +178,9 @@ bool DiveSiteSortedModel::filterAcceptsRow(int sourceRow, const QModelIndex &sou if (fullText.isEmpty()) return true; - if (sourceRow < 0 || sourceRow > divelog.sites->nr) + if (sourceRow < 0 || sourceRow > (int)divelog.sites->size()) return false; - struct dive_site *ds = divelog.sites->dive_sites[sourceRow]; + const auto &ds = (*divelog.sites)[sourceRow]; QString text = QString::fromStdString(ds->name + ds->description + ds->notes); return text.contains(fullText, Qt::CaseInsensitive); } @@ -193,10 +190,15 @@ bool DiveSiteSortedModel::lessThan(const QModelIndex &i1, const QModelIndex &i2) // The source indices correspond to indices in the global dive site table. // Let's access them directly without going via the source model. // Kind of dirty, but less effort. - struct dive_site *ds1 = get_dive_site(i1.row(), divelog.sites); - struct dive_site *ds2 = get_dive_site(i2.row(), divelog.sites); - if (!ds1 || !ds2) // Invalid dive sites compare as different - return false; + + // Be careful to respect proper ordering when sites are invalid. + bool valid1 = i1.row() >= 0 && i1.row() < (int)divelog.sites->size(); + bool valid2 = i2.row() >= 0 && i2.row() < (int)divelog.sites->size(); + if (!valid1 || !valid2) + return valid1 < valid2; + + const auto &ds1 = (*divelog.sites)[i1.row()]; + const auto &ds2 = (*divelog.sites)[i2.row()]; switch (i1.column()) { case LocationInformationModel::NAME: default: @@ -230,18 +232,19 @@ QStringList DiveSiteSortedModel::allSiteNames() const // This shouldn't happen, but if model and core get out of sync, // (more precisely: the core has more sites than the model is aware of), // we might get an invalid index. - if (idx < 0 || idx > divelog.sites->nr) { + if (idx < 0 || idx > (int)divelog.sites->size()) { report_info("DiveSiteSortedModel::allSiteNames(): invalid index"); continue; } - locationNames << QString::fromStdString(divelog.sites->dive_sites[idx]->name); + locationNames << QString::fromStdString((*divelog.sites)[idx]->name); } return locationNames; } -struct dive_site *DiveSiteSortedModel::getDiveSite(const QModelIndex &idx) +struct dive_site *DiveSiteSortedModel::getDiveSite(const QModelIndex &idx_source) { - return get_dive_site(mapToSource(idx).row(), divelog.sites); + auto idx = mapToSource(idx_source).row(); + return idx >= 0 && idx < (int)divelog.sites->size() ? (*divelog.sites)[idx].get() : NULL; } #ifndef SUBSURFACE_MOBILE diff --git a/qt-models/divelocationmodel.h b/qt-models/divelocationmodel.h index 57471dee6..adcd425c5 100644 --- a/qt-models/divelocationmodel.h +++ b/qt-models/divelocationmodel.h @@ -19,7 +19,7 @@ public: // Thus, different views can connect to different models. enum Columns { EDIT, REMOVE, NAME, DESCRIPTION, NUM_DIVES, LOCATION, NOTES, DIVESITE, TAXONOMY, COLUMNS }; enum Roles { DIVESITE_ROLE = Qt::UserRole + 1 }; - static QVariant getDiveSiteData(const struct dive_site *ds, int column, int role); + static QVariant getDiveSiteData(const struct dive_site &ds, int column, int role); LocationInformationModel(QObject *obj = 0); static LocationInformationModel *instance(); diff --git a/qt-models/divesiteimportmodel.cpp b/qt-models/divesiteimportmodel.cpp index 363a4e7b4..089a787bf 100644 --- a/qt-models/divesiteimportmodel.cpp +++ b/qt-models/divesiteimportmodel.cpp @@ -50,9 +50,9 @@ QVariant DivesiteImportedModel::data(const QModelIndex &index, int role) const if (index.row() + firstIndex > lastIndex) return QVariant(); - struct dive_site *ds = get_dive_site(index.row() + firstIndex, importedSitesTable); - if (!ds) + if (index.row() < 0 || index.row() >= (int)importedSitesTable->size()) return QVariant(); + struct dive_site *ds = (*importedSitesTable)[index.row()].get(); // widgets access the model via index.column() // Not supporting QML access via roles @@ -68,7 +68,7 @@ QVariant DivesiteImportedModel::data(const QModelIndex &index, int role) const // 40075000 is circumference of the earth in meters struct dive_site *nearest_ds = get_dive_site_by_gps_proximity(&ds->location, - 40075000, divelog.sites); + 40075000, *divelog.sites); if (nearest_ds) return QString::fromStdString(nearest_ds->name); else @@ -78,7 +78,7 @@ QVariant DivesiteImportedModel::data(const QModelIndex &index, int role) const unsigned int distance = 0; struct dive_site *nearest_ds = get_dive_site_by_gps_proximity(&ds->location, - 40075000, divelog.sites); + 40075000, *divelog.sites); if (nearest_ds) distance = get_distance(&ds->location, &nearest_ds->location); @@ -126,18 +126,15 @@ Qt::ItemFlags DivesiteImportedModel::flags(const QModelIndex &index) const return QAbstractTableModel::flags(index) | Qt::ItemIsUserCheckable; } -void DivesiteImportedModel::repopulate(struct dive_site_table *sites) +void DivesiteImportedModel::repopulate(dive_site_table *sites) { beginResetModel(); importedSitesTable = sites; firstIndex = 0; - lastIndex = importedSitesTable->nr - 1; - checkStates.resize(importedSitesTable->nr); - for (int row = 0; row < importedSitesTable->nr; row++) - if (get_dive_site_by_gps(&importedSitesTable->dive_sites[row]->location, divelog.sites)) - checkStates[row] = false; - else - checkStates[row] = true; + lastIndex = (int)importedSitesTable->size() - 1; // Qt: the "last index" is negative for empty lists. Insane. + checkStates.resize(importedSitesTable->size()); + for (size_t row = 0; row < importedSitesTable->size(); row++) + checkStates[row] = !get_dive_site_by_gps(&(*importedSitesTable)[row]->location, *divelog.sites); endResetModel(); } diff --git a/qt-models/divesiteimportmodel.h b/qt-models/divesiteimportmodel.h index 5c7fb27fe..8b1b13b20 100644 --- a/qt-models/divesiteimportmodel.h +++ b/qt-models/divesiteimportmodel.h @@ -17,7 +17,7 @@ public: QVariant data(const QModelIndex& index, int role) const; QVariant headerData(int section, Qt::Orientation orientation, int role) const; Qt::ItemFlags flags(const QModelIndex &index) const; - void repopulate(dive_site_table_t *sites); + void repopulate(dive_site_table *sites); public slots: void changeSelected(QModelIndex clickedIndex); @@ -29,7 +29,7 @@ private: int firstIndex; int lastIndex; std::vector checkStates; // char instead of bool to avoid silly pessimization of std::vector. - struct dive_site_table *importedSitesTable; + dive_site_table *importedSitesTable; }; #endif diff --git a/qt-models/maplocationmodel.cpp b/qt-models/maplocationmodel.cpp index 82ced53ba..8534e0d38 100644 --- a/qt-models/maplocationmodel.cpp +++ b/qt-models/maplocationmodel.cpp @@ -118,15 +118,15 @@ const QVector &MapLocationModel::selectedDs() const return m_selectedDs; } -static bool hasVisibleDive(const dive_site *ds) +static bool hasVisibleDive(const dive_site &ds) { - return std::any_of(ds->dives.begin(), ds->dives.end(), + return std::any_of(ds.dives.begin(), ds.dives.end(), [] (const dive *d) { return !d->hidden_by_filter; }); } -static bool hasSelectedDive(const dive_site *ds) +static bool hasSelectedDive(const dive_site &ds) { - return std::any_of(ds->dives.begin(), ds->dives.end(), + return std::any_of(ds.dives.begin(), ds.dives.end(), [] (const dive *d) { return d->selected; }); } @@ -160,18 +160,17 @@ void MapLocationModel::reload(QObject *map) if (diveSiteMode) m_selectedDs = DiveFilter::instance()->filteredDiveSites(); #endif - for (int i = 0; i < divelog.sites->nr; ++i) { - struct dive_site *ds = divelog.sites->dive_sites[i]; + for (const auto &ds: *divelog.sites) { QGeoCoordinate dsCoord; // Don't show dive sites of hidden dives, unless we're in dive site edit mode. - if (!diveSiteMode && !hasVisibleDive(ds)) + if (!diveSiteMode && !hasVisibleDive(*ds)) continue; - if (!dive_site_has_gps_location(ds)) { + if (!dive_site_has_gps_location(ds.get())) { // Dive sites that do not have a gps location are not shown in normal mode. // In dive-edit mode, selected sites are placed at the center of the map, // so that the user can drag them somewhere without having to enter coordinates. - if (!diveSiteMode || !m_selectedDs.contains(ds) || !map) + if (!diveSiteMode || !m_selectedDs.contains(ds.get()) || !map) continue; dsCoord = map->property("center").value(); } else { @@ -179,8 +178,8 @@ void MapLocationModel::reload(QObject *map) qreal longitude = ds->location.lon.udeg * 0.000001; dsCoord = QGeoCoordinate(latitude, longitude); } - if (!diveSiteMode && hasSelectedDive(ds) && !m_selectedDs.contains(ds)) - m_selectedDs.append(ds); + if (!diveSiteMode && hasSelectedDive(*ds) && !m_selectedDs.contains(ds.get())) + m_selectedDs.append(ds.get()); QString name = siteMapDisplayName(ds->name); if (!diveSiteMode) { // don't add dive locations with the same name, unless they are @@ -192,8 +191,8 @@ void MapLocationModel::reload(QObject *map) continue; } } - bool selected = m_selectedDs.contains(ds); - MapLocation *location = new MapLocation(ds, dsCoord, name, selected); + bool selected = m_selectedDs.contains(ds.get()); + MapLocation *location = new MapLocation(ds.get(), dsCoord, name, selected); m_mapLocations.append(location); if (!diveSiteMode) locationNameMap[name] = location; diff --git a/smtk-import/smartrak.cpp b/smtk-import/smartrak.cpp index d8ff9258b..4b80f1a3a 100644 --- a/smtk-import/smartrak.cpp +++ b/smtk-import/smartrak.cpp @@ -423,12 +423,12 @@ static void smtk_build_location(MdbHandle *mdb, char *idx, struct dive_site **lo concat(str, ", ", table.get_string_view(1)); // Locality concat(str, ", ", site); - ds = get_dive_site_by_name(str, log->sites); + ds = get_dive_site_by_name(str, *log->sites); if (!ds) { if (!has_location(&loc)) - ds = create_dive_site(str, log->sites); + ds = create_dive_site(str, *log->sites); else - ds = create_dive_site_with_gps(str, &loc, log->sites); + ds = create_dive_site_with_gps(str, &loc, *log->sites); } *location = ds; diff --git a/tests/testdivesiteduplication.cpp b/tests/testdivesiteduplication.cpp index e905e3ee4..af5e798ac 100644 --- a/tests/testdivesiteduplication.cpp +++ b/tests/testdivesiteduplication.cpp @@ -9,7 +9,7 @@ void TestDiveSiteDuplication::testReadV2() { prefs.cloud_base_url = strdup(default_prefs.cloud_base_url); QCOMPARE(parse_file(SUBSURFACE_TEST_DATA "/dives/TwoTimesTwo.ssrf", &divelog), 0); - QCOMPARE(divelog.sites->nr, 2); + QCOMPARE(divelog.sites->size(), 2); } QTEST_GUILESS_MAIN(TestDiveSiteDuplication) diff --git a/tests/testparse.cpp b/tests/testparse.cpp index 2d574b7a5..7814281c0 100644 --- a/tests/testparse.cpp +++ b/tests/testparse.cpp @@ -92,7 +92,7 @@ int TestParse::parseCSV(int units, std::string file) int TestParse::parseDivingLog() { // Parsing of DivingLog import from SQLite database - struct dive_site *ds = alloc_or_get_dive_site(0xdeadbeef, divelog.sites); + struct dive_site *ds = alloc_or_get_dive_site(0xdeadbeef, *divelog.sites); ds->name = "Suomi - - Hälvälä"; int ret = sqlite3_open(SUBSURFACE_TEST_DATA "/dives/TestDivingLog4.1.1.sql", &_sqlite3_handle);