core: use C++-primitives for g_tag_list

The old code was leaking memory. Use std::unique_ptr<> for
ownership management.

This is still very primitive and divetags are kept during
application lifetime. There should probably be some form
of reference counting. And the taglist should not be global,
but attached to the divelog.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

backend-shared/exportfuncs.cpp

@@ -191,14 +191,8 @@ void export_TeX(const char *filename, bool selected_only, bool plain, ExportCall
 		dive->maxdepth.mm ? put_format(&buf, "\\def\\%smaximumdepth{%.1f\\%sdepthunit}\n", ssrf, get_depth_units(dive->maxdepth.mm, NULL, &unit), ssrf) : put_format(&buf, "\\def\\%smaximumdepth{}\n", ssrf);
 		dive->meandepth.mm ? put_format(&buf, "\\def\\%smeandepth{%.1f\\%sdepthunit}\n", ssrf, get_depth_units(dive->meandepth.mm, NULL, &unit), ssrf) : put_format(&buf, "\\def\\%smeandepth{}\n", ssrf);
 
-		struct tag_entry *tag = dive->tag_list;
-		QString tags;
-		if (tag) {
-			tags = tag->tag->name;
-			while ((tag = tag->next))
-				tags += QString(", ") + QString(tag->tag->name);
-		}
-		put_format(&buf, "\\def\\%stype{%s}\n", ssrf, qPrintable(tags));
+		std::string tags = taglist_get_tagstring(dive->tag_list);
+		put_format(&buf, "\\def\\%stype{%s}\n", ssrf, tags.c_str());
 		put_format(&buf, "\\def\\%sviz{%s}\n", ssrf, qPrintable(viz));
 		put_format(&buf, "\\def\\%srating{%s}\n", ssrf, qPrintable(rating));
 		put_format(&buf, "\\def\\%splot{\\includegraphics[width=9cm,height=4cm]{profile%d}}\n", ssrf, dive->number);

commands/command_edit.cpp

@@ -566,7 +566,7 @@ QStringList EditTags::data(struct dive *d) const
 {
 	QStringList res;
 	for (const struct tag_entry *tag = d->tag_list; tag; tag = tag->next)
-		res.push_back(tag->tag->name);
+		res.push_back(QString::fromStdString(tag->tag->name));
 	return res;
 }
 
@@ -1426,7 +1426,7 @@ EditDive::EditDive(dive *oldDiveIn, dive *newDiveIn, dive_site *createDs, dive_s
 		changedFields |= DiveField::CHILL;
 	if (!same_string(oldDive->suit, newDive->suit))
 		changedFields |= DiveField::SUIT;
-	if (get_taglist_string(oldDive->tag_list) != get_taglist_string(newDive->tag_list)) // This is cheating. Do we have a taglist comparison function?
+	if (taglist_get_tagstring(oldDive->tag_list) != taglist_get_tagstring(newDive->tag_list)) // This is cheating. Do we have a taglist comparison function?
 		changedFields |= DiveField::TAGS;
 	if (oldDive->dc.divemode != newDive->dc.divemode)
 		changedFields |= DiveField::MODE;

core/datatrak.cpp

@@ -721,7 +721,6 @@ int datatrak_import(std::string &mem, std::string &wl_mem, struct divelog *log)
 		i++;
 	}
 out:
-	taglist_cleanup(&g_tag_list);
 	sort_dive_table(log->dives);
 	return rc;
 bail:

core/filterconstraint.cpp

@@ -819,7 +819,7 @@ static bool has_tags(const filter_constraint &c, const struct dive *d)
 {
 	QStringList dive_tags;
 	for (const tag_entry *tag = d->tag_list; tag; tag = tag->next)
-		dive_tags.push_back(QString(tag->tag->name).trimmed());
+		dive_tags.push_back(QString::fromStdString(tag->tag->name).trimmed());
 	dive_tags.append(gettextFromC::tr(divemode_text_ui[d->dc.divemode]).trimmed());
 	return check(c, dive_tags);
 }

core/fulltext.cpp

@@ -128,7 +128,7 @@ static std::vector<QString> getWords(const dive *d)
 	tokenize(QString(d->buddy), res);
 	tokenize(QString(d->suit), res);
 	for (const tag_entry *tag = d->tag_list; tag; tag = tag->next)
-		tokenize(QString(tag->tag->name), res);
+		tokenize(QString::fromStdString(tag->tag->name), res);
 	for (int i = 0; i < d->cylinders.nr; ++i) {
 		const cylinder_t &cyl = *get_cylinder(d, i);
 		tokenize(QString(cyl.type.description), res);

core/qthelper.cpp

@@ -1229,12 +1229,6 @@ QStringList get_dive_gas_list(const struct dive *d)
 	return list;
 }
 
-QString get_taglist_string(struct tag_entry *tag_list)
-{
-	std::string tags = taglist_get_tagstring(tag_list);
-	return QString::fromStdString(tags);
-}
-
 QStringList stringToList(const QString &s)
 {
 	QStringList res = s.split(",", SKIP_EMPTY);

core/qthelper.h

@@ -36,7 +36,6 @@ QString distance_string(int distanceInMeters);
 bool gpsHasChanged(struct dive *dive, struct dive *master, const QString &gps_text, bool *parsed_out = 0);
 QString get_gas_string(struct gasmix gas);
 QStringList get_dive_gas_list(const struct dive *d);
-QString get_taglist_string(struct tag_entry *tag_list);
 QStringList stringToList(const QString &s);
 void read_hashes();
 void write_hashes();

core/save-git.cpp

@@ -113,7 +113,7 @@ static void save_tags(struct membuffer *b, struct tag_entry *tags)
 		return;
 	put_string(b, "tags");
 	while (tags) {
-		show_utf8(b, sep, tags->tag->source ? : tags->tag->name, "");
+		show_utf8(b, sep, tags->tag->source.empty() ? tags->tag->name.c_str() : tags->tag->source.c_str(), "");
 		sep = ", ";
 		tags = tags->next;
 	}

core/save-html.cpp

@@ -339,7 +339,7 @@ static void put_HTML_tags(struct membuffer *b, struct dive *dive, const char *pr
 	while (tag) {
 		put_format(b, "%s\"", separator);
 		separator = ", ";
-		put_HTML_quoted(b, tag->tag->name);
+		put_HTML_quoted(b, tag->tag->name.c_str());
 		put_string(b, "\"");
 		tag = tag->next;
 	}

core/save-xml.cpp

@@ -387,10 +387,10 @@ static void save_tags(struct membuffer *b, struct tag_entry *entry)
 	if (entry) {
 		const char *sep = " tags='";
 		do {
-			struct divetag *tag = entry->tag;
+			const struct divetag *tag = entry->tag;
 			put_string(b, sep);
 			/* If the tag has been translated, write the source to the xml file */
-			quote(b, tag->source ?: tag->name, 1);
+			quote(b, tag->source.empty() ? tag->name.c_str() : tag->source.c_str(), 1);
 			sep = ", ";
 		} while ((entry = entry->next) != NULL);
 		put_string(b, "'");

core/tag.cpp

@@ -7,9 +7,10 @@
 #include "gettext.h"
 
 #include <stdlib.h>
+#include <algorithm>
 #include <QtGlobal> // for QT_TRANSLATE_NOOP
 
-struct tag_entry *g_tag_list = NULL;
+std::vector<std::unique_ptr<divetag>> g_tag_list;
 
 static const char *default_tags[] = {
 	QT_TRANSLATE_NOOP("gettextFromC", "boat"), QT_TRANSLATE_NOOP("gettextFromC", "shore"), QT_TRANSLATE_NOOP("gettextFromC", "drift"),
@@ -24,15 +25,13 @@ static const char *default_tags[] = {
 /* copy an element in a list of tags */
 static void copy_tl(struct tag_entry *st, struct tag_entry *dt)
 {
-	dt->tag = (divetag *)malloc(sizeof(struct divetag));
-	dt->tag->name = copy_string(st->tag->name);
-	dt->tag->source = copy_string(st->tag->source);
+	dt->tag = st->tag;
 }
 
 static bool tag_seen_before(struct tag_entry *start, struct tag_entry *before)
 {
 	while (start && start != before) {
-		if (same_string(start->tag->name, before->tag->name))
+		if (start->tag->name == before->tag->name)
 			return true;
 		start = start->next;
 	}
@@ -45,7 +44,7 @@ extern "C" void taglist_cleanup(struct tag_entry **tag_list)
 	struct tag_entry **tl = tag_list;
 	while (*tl) {
 		/* skip tags that are empty or that we have seen before */
-		if (empty_string((*tl)->tag->name) || tag_seen_before(*tag_list, *tl)) {
+		if ((*tl)->tag->name.empty() || tag_seen_before(*tag_list, *tl)) {
 			*tl = (*tl)->next;
 			continue;
 		}
@@ -57,39 +56,28 @@ std::string taglist_get_tagstring(struct tag_entry *tag_list)
 {
 	bool first_tag = true;
 	std::string res;
-	struct tag_entry *tmp = tag_list;
-	while (tmp != NULL) {
-		if (!empty_string(tmp->tag->name)) {
-			if (!first_tag)
-				res += ", ";
-			res += tmp->tag->name;
-			first_tag = false;
-		}
-		tmp = tmp->next;
+	for (struct tag_entry *tmp = tag_list; tmp != NULL; tmp = tmp->next) {
+		if (tmp->tag->name.empty())
+			continue;
+		if (!first_tag)
+			res += ", ";
+		res += tmp->tag->name;
+		first_tag = false;
 	}
-	return strdup(res.c_str());
-}
-
-static inline void taglist_free_divetag(struct divetag *tag)
-{
-	if (tag->name != NULL)
-		free(tag->name);
-	if (tag->source != NULL)
-		free(tag->source);
-	free(tag);
+	return res;
 }
 
 /* Add a tag to the tag_list, keep the list sorted */
-static struct divetag *taglist_add_divetag(struct tag_entry **tag_list, struct divetag *tag)
+static void taglist_add_divetag(struct tag_entry **tag_list, const struct divetag *tag)
 {
 	struct tag_entry *next, *entry;
 
 	while ((next = *tag_list) != NULL) {
-		int cmp = strcmp(next->tag->name, tag->name);
+		int cmp = next->tag->name.compare(tag->name);
 
 		/* Already have it? */
 		if (!cmp)
-			return next->tag;
+			return;
 		/* Is the entry larger? If so, insert here */
 		if (cmp > 0)
 			break;
@@ -102,48 +90,33 @@ static struct divetag *taglist_add_divetag(struct tag_entry **tag_list, struct d
 	entry->next = next;
 	entry->tag = tag;
 	*tag_list = entry;
-	return tag;
 }
 
-extern "C" struct divetag *taglist_add_tag(struct tag_entry **tag_list, const char *tag)
+static const divetag *register_tag(const char *s, const char *source)
 {
-	size_t i = 0;
-	int is_default_tag = 0;
-	struct divetag *ret_tag, *new_tag;
-	const char *translation;
-	new_tag = (divetag *)malloc(sizeof(struct divetag));
+	// binary search
+	auto it = std::lower_bound(g_tag_list.begin(), g_tag_list.end(), s,
+				   [](const std::unique_ptr<divetag> &tag, const char *s)
+				   { return tag->name < s; });
+	if (it == g_tag_list.end() || (*it)->name != s) {
+		std::string source_s = empty_string(source) ? std::string() : std::string(source);
+		it = g_tag_list.insert(it, std::make_unique<divetag>(s, source));
+	}
+	return it->get();
+}
+
+extern "C" void taglist_add_tag(struct tag_entry **tag_list, const char *tag)
+{
+	bool is_default_tag = std::find_if(std::begin(default_tags), std::end(default_tags),
+			[&tag] (const char *default_tag) { return tag == default_tag; });
 
-	for (i = 0; i < std::size(default_tags); i++) {
-		if (strcmp(default_tags[i], tag) == 0) {
-			is_default_tag = 1;
-			break;
-		}
-	}
 	/* Only translate default tags */
-	if (is_default_tag) {
-		translation = translate("gettextFromC", tag);
-		new_tag->name = (char *)malloc(strlen(translation) + 1);
-		memcpy(new_tag->name, translation, strlen(translation) + 1);
-		new_tag->source = (char *)malloc(strlen(tag) + 1);
-		memcpy(new_tag->source, tag, strlen(tag) + 1);
-	} else {
-		new_tag->source = NULL;
-		new_tag->name = (char *)malloc(strlen(tag) + 1);
-		memcpy(new_tag->name, tag, strlen(tag) + 1);
-	}
-	/* Try to insert new_tag into g_tag_list if we are not operating on it */
-	if (tag_list != &g_tag_list) {
-		ret_tag = taglist_add_divetag(&g_tag_list, new_tag);
-		/* g_tag_list already contains new_tag, free the duplicate */
-		if (ret_tag != new_tag)
-			taglist_free_divetag(new_tag);
-		ret_tag = taglist_add_divetag(tag_list, ret_tag);
-	} else {
-		ret_tag = taglist_add_divetag(tag_list, new_tag);
-		if (ret_tag != new_tag)
-			taglist_free_divetag(new_tag);
-	}
-	return ret_tag;
+	/* TODO: Do we really want to translate user-supplied tags if they happen to be known!? */
+	const char *translation = is_default_tag ? translate("gettextFromC", tag) : tag;
+	const char *source = is_default_tag ? tag : nullptr;
+	const struct divetag *d_tag = register_tag(translation, source);
+
+	taglist_add_divetag(tag_list, d_tag);
 }
 
 extern "C" void taglist_free(struct tag_entry *entry)
@@ -171,8 +144,6 @@ extern "C" void taglist_merge(struct tag_entry **dst, struct tag_entry *src1, st
 
 extern "C" void taglist_init_global()
 {
-	size_t i;
-
-	for (i = 0; i < std::size(default_tags); i++)
-		taglist_add_tag(&g_tag_list, default_tags[i]);
+	for (const char *s: default_tags)
+		register_tag(translate("gettextFromC", s), s);
 }

core/tag.h

@@ -6,35 +6,37 @@
 #include <stdbool.h>
 
 #ifdef __cplusplus
+#include <memory>
+#include <string>
+#include <vector>
+
 extern "C" {
 #endif
 
 struct divetag {
+#ifdef __cplusplus
 	/*
 	 * The name of the divetag. If a translation is available, name contains
 	 * the translated tag
 	 */
-	char *name;
+	std::string name;
 	/*
 	 * If a translation is available, we write the original tag to source.
 	 * This enables us to write a non-localized tag to the xml file.
 	 */
-	char *source;
+	std::string source;
+	divetag(const char *n, const char *s) : name(n), source(s)
+	{
+	}
+#endif
 };
 
 struct tag_entry {
-	struct divetag *tag;
+	const struct divetag *tag;
 	struct tag_entry *next;
 };
 
-/*
- * divetags are only stored once, each dive only contains
- * a list of tag_entries which then point to the divetags
- * in the global g_tag_list
- */
-extern struct tag_entry *g_tag_list;
-
-struct divetag *taglist_add_tag(struct tag_entry **tag_list, const char *tag);
+void taglist_add_tag(struct tag_entry **tag_list, const char *tag);
 
 /* cleans up a list: removes empty tags and duplicates */
 void taglist_cleanup(struct tag_entry **tag_list);
@@ -45,6 +47,21 @@ struct tag_entry *taglist_copy(struct tag_entry *s);
 void taglist_merge(struct tag_entry **dst, struct tag_entry *src1, struct tag_entry *src2);
 
 #ifdef __cplusplus
+
+/*
+ * divetags are only stored once, each dive only contains
+ * a list of tag_entries which then point to the divetags
+ * in the global g_tag_list
+ */
+extern std::vector<std::unique_ptr<divetag>> g_tag_list;
+
+/*
+ * Writes all divetags form tag_list into internally allocated buffer
+ * Function returns pointer to allocated buffer
+ * Buffer contains comma separated list of tags names or null terminated string
+ */
+extern std::string taglist_get_tagstring(struct tag_entry *tag_list);
+
 }
 
 // C++ only functions

desktop-widgets/simplewidgets.cpp

@@ -342,7 +342,7 @@ void DiveComponentSelection::buttonClicked(QAbstractButton *button)
 			text << tr("Tags: ");
 			tag_entry *entry = current_dive->tag_list;
 			while (entry) {
-				text << entry->tag->name << " ";
+				text << entry->tag->name.c_str() << " ";
 				entry = entry->next;
 			}
 			text << "\n";

desktop-widgets/tab-widgets/TabDiveNotes.cpp

@@ -5,6 +5,7 @@
 #include "core/qthelper.h"
 #include "core/selection.h"
 #include "core/subsurface-string.h"
+#include "core/tag.h"
 #include "core/trip.h"
 #include "desktop-widgets/mainwindow.h"
 #include "desktop-widgets/mapwidget.h"
@@ -117,7 +118,7 @@ void TabDiveNotes::divesChanged(const QVector<dive *> &dives, DiveField field)
 	if (field.divesite)
 		updateDiveSite(currentDive);
 	if (field.tags)
-		ui.tagWidget->setText(get_taglist_string(currentDive->tag_list));
+		ui.tagWidget->setText(QString::fromStdString(taglist_get_tagstring(currentDive->tag_list)));
 	if (field.buddy)
 		ui.buddy->setText(currentDive->buddy);
 	if (field.diveguide)
@@ -252,7 +253,7 @@ void TabDiveNotes::updateData(const std::vector<dive *> &, dive *currentDive, in
 		// reset labels in case we last displayed trip notes
 		ui.LocationLabel->setText(tr("Location"));
 		ui.NotesLabel->setText(tr("Notes"));
-		ui.tagWidget->setText(get_taglist_string(currentDive->tag_list));
+		ui.tagWidget->setText(QString::fromStdString(taglist_get_tagstring(currentDive->tag_list)));
 		bool isManual = is_manually_added_dc(&currentDive->dc);
 		ui.depth->setVisible(isManual);
 		ui.depthLabel->setVisible(isManual);

desktop-widgets/templatelayout.cpp

@@ -10,6 +10,7 @@
 #include "printoptions.h"
 #include "core/divelist.h"
 #include "core/selection.h"
+#include "core/tag.h"
 #include "core/qthelper.h"
 #include "core/string-format.h"
 
@@ -552,7 +553,7 @@ QVariant TemplateLayout::getValue(QString list, QString property, const State &s
 		} else if (property == "notes") {
 			return formatNotes(d);
 		} else if (property == "tags") {
-			return get_taglist_string(d->tag_list);
+			return QString::fromStdString(taglist_get_tagstring(d->tag_list));
 		} else if (property == "gas") {
 			return formatGas(d);
 		} else if (property == "sac") {

mobile-widgets/qmlmanager.cpp

@@ -1325,7 +1325,7 @@ void QMLManager::commitChanges(QString diveId, QString number, QString date, QSt
 	}
 	// normalize the tag list we have and the one we get from the UI
 	// try hard to deal with accidental white space issues
-	QStringList existingTagList = get_taglist_string(d->tag_list).split(",", SKIP_EMPTY);
+	QStringList existingTagList = QString::fromStdString(taglist_get_tagstring(d->tag_list)).split(",", SKIP_EMPTY);
 	QStringList newTagList = tags.split(",", SKIP_EMPTY);
 	QStringList newCleanTagList;
 	for (QString s: newTagList) {

qt-models/completionmodels.cpp

@@ -84,14 +84,9 @@ bool SuitCompletionModel::relevantDiveField(const DiveField &f)
 
 QStringList TagCompletionModel::getStrings()
 {
-	if (g_tag_list == NULL)
-		return {};
 	QStringList list;
-	struct tag_entry *current_tag_entry = g_tag_list;
-	while (current_tag_entry != NULL) {
-		list.append(QString(current_tag_entry->tag->name));
-		current_tag_entry = current_tag_entry->next;
-	}
+	for (const std::unique_ptr<divetag> &tag: g_tag_list)
+		list.append(QString::fromStdString(tag->name));
 	std::sort(list.begin(), list.end());
 	return list;
 }

qt-models/divetripmodel.cpp

@@ -294,7 +294,7 @@ QVariant DiveTripModelBase::diveData(const struct dive *d, int column, int role)
 	case MobileListModel::SumWeightRole: return formatSumWeight(d);
 	case MobileListModel::DiveGuideRole: return QString(d->diveguide);
 	case MobileListModel::BuddyRole: return QString(d->buddy);
-	case MobileListModel::TagsRole: return get_taglist_string(d->tag_list);
+	case MobileListModel::TagsRole: return QString::fromStdString(taglist_get_tagstring(d->tag_list));
 	case MobileListModel::NotesRole: return formatNotes(d);
 	case MobileListModel::GpsRole: return formatDiveGPS(d);
 	case MobileListModel::GpsDecimalRole: return format_gps_decimal(d);
@@ -347,7 +347,7 @@ QVariant DiveTripModelBase::diveData(const struct dive *d, int column, int role)
 			else
 				return d->maxcns;
 		case TAGS:
-			return get_taglist_string(d->tag_list);
+			return QString::fromStdString(taglist_get_tagstring(d->tag_list));
 		case PHOTOS:
 			break;
 		case COUNTRY:

stats/statsvariables.cpp

@@ -1502,7 +1502,7 @@ struct TagBinner : public StringBinner<TagBinner, StringBin> {
 	std::vector<QString> to_bin_values(const dive *d) const {
 		std::vector<QString> tags;
 		for (const tag_entry *tag = d->tag_list; tag; tag = tag->next)
-			tags.push_back(QString(tag->tag->name).trimmed());
+			tags.push_back(QString::fromStdString(tag->tag->name).trimmed());
 		return tags;
 	}
 };
@@ -1513,7 +1513,7 @@ struct TagVariable : public StatsVariableTemplate<StatsVariable::Type::Discrete>
 		return StatsTranslations::tr("Tags");
 	}
 	QString diveCategories(const dive *d) const override {
-		return get_taglist_string(d->tag_list);
+		return QString::fromStdString(taglist_get_tagstring(d->tag_list));
 	}
 	std::vector<const StatsBinner *> binners() const override {
 		return { &tag_binner };

subsurface-desktop-main.cpp

@@ -107,7 +107,6 @@ int main(int argc, char **argv)
 		run_ui();
 	exit_ui();
 	clear_divelog(&divelog);
-	taglist_free(g_tag_list);
 	parse_xml_exit();
 	subsurface_console_exit();
 

subsurface-downloader-main.cpp

@@ -110,7 +110,6 @@ int main(int argc, char **argv)
 		printf("Give a log file name as argument, or configure a cloud URL.\n");
 	}
 	clear_divelog(&divelog);
-	taglist_free(g_tag_list);
 	parse_xml_exit();
 
 	// Sync struct preferences to disk

subsurface-mobile-main.cpp

@@ -94,7 +94,6 @@ int main(int argc, char **argv)
 		run_mobile_ui(initial_font_size);
 	exit_ui();
 	clear_divelog(&divelog);
-	taglist_free(g_tag_list);
 	parse_xml_exit();
 	subsurface_console_exit();
 

tests/testtaglist.cpp

@@ -10,8 +10,7 @@ void TestTagList::initTestCase()
 
 void TestTagList::cleanupTestCase()
 {
-	taglist_free(g_tag_list);
-	g_tag_list = NULL;
+	g_tag_list.clear();
 }
 
 void TestTagList::testGetTagstringNoTags()