From a3499799c13957e9528f4e97db3a93273eab6212 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Fri, 11 Dec 2015 18:07:14 +0000 Subject: [PATCH] Don't manually manage API outputs' memory Using the stack will greatly simplify using thread-local storage for thread safety. Also take the opportunity to encapsulate the data members affected, which is partly to prevent desynchronisation between the collection pairs used, and partly also preparation for thread safety work. --- src/api/api.cpp | 368 ++++++++++++++++++++++-------------------------- 1 file changed, 171 insertions(+), 197 deletions(-) diff --git a/src/api/api.cpp b/src/api/api.cpp index 8083c898..7a9f7971 100644 --- a/src/api/api.cpp +++ b/src/api/api.cpp @@ -91,82 +91,161 @@ const unsigned int loot_needs_cleaning_unknown = 2; struct _loot_db_int : public loot::Game { _loot_db_int(const unsigned int clientGame, const std::string& gamePath, const boost::filesystem::path& gameLocalDataPath) - : Game(clientGame), - extTagMap(nullptr), - extAddedTagIds(nullptr), - extRemovedTagIds(nullptr), - extMessageArray(nullptr), - extMessageArraySize(0), - extStringArray(nullptr), - extStringArraySize(0), - extRevisionID(nullptr), - extRevisionDate(nullptr) { + : Game(clientGame) { this->SetGamePath(gamePath); this->Init(false, gameLocalDataPath); } - ~_loot_db_int() { - delete[] extAddedTagIds; - delete[] extRemovedTagIds; - delete[] extRevisionID; - delete[] extRevisionDate; - - if (extTagMap != nullptr) { - for (size_t i = 0; i < bashTagMap.size(); i++) - delete[] extTagMap[i]; //Gotta clear those allocated strings. - delete[] extTagMap; - } - - if (extMessageArray != nullptr) { - for (size_t i = 0; i < extMessageArraySize; i++) - delete[] extMessageArray[i].message; //Gotta clear those allocated strings. - delete[] extMessageArray; - } - - if (extStringArray != nullptr) { - for (size_t i = 0; i < extStringArraySize; i++) - delete[] extStringArray[i]; //Gotta clear those allocated strings. - delete[] extStringArray; - } - } - loot::MetadataList rawUserMetadata; loot::Masterlist rawMetadata; + const char * getRevisionIdString() const { + return revisionId.c_str(); + } + + const char * getRevisionDateString() const { + return revisionDate.c_str(); + } + + const std::vector& getPluginNames() const { + return cPluginNames; + } + + const std::vector& getBashTagMap() const { + return cBashTagMap; + } + + unsigned int getBashTagUid(const std::string& name) const { + auto it = bashTagMap.find(name); + if (it != end(bashTagMap)) + return it->second; + + throw loot::error(loot::error::no_tag_map, "The Bash Tag \"" + name + "\" does not exist in the Bash Tag map."); + } + + const std::vector& getAddedTagIds() const { + return addedTagIds; + } + + const std::vector& getRemovedTagIds() const { + return removedTagIds; + } + + const std::vector& getPluginMessages() const { + return cPluginMessages; + } + + void setRevisionIdString(const std::string& str) { + revisionId = str; + } + + void setRevisionDateString(const std::string& str) { + revisionDate = str; + } + + template + void setPluginNames(const T& plugins) { + // First take copies of the C++ strings to store. + pluginNames.resize(plugins.size()); + std::transform(begin(plugins), + end(plugins), + begin(pluginNames), + [](const loot::Plugin& plugin) { + return plugin.Name(); + }); + + // Now store their C strings. + cPluginNames.resize(pluginNames.size()); + std::transform(begin(pluginNames), + end(pluginNames), + begin(cPluginNames), + [](const std::string& pluginName) { + return pluginName.c_str(); + }); + } + + template + void setAddedTags(const T& names) { + for (const auto& name : names) + addedTagIds.push_back(getBashTagUid(name)); + } + + template + void setRemovedTags(const T& names) { + for (const auto& name : names) + removedTagIds.push_back(getBashTagUid(name)); + } + + template + void setPluginMessages(const T& pluginMessages) { + cPluginMessages.resize(pluginMessages.size()); + pluginMessageStrings.resize(pluginMessages.size()); + + size_t i = 0; + for (const auto& message : pluginMessages) { + pluginMessageStrings[i] = message.ChooseContent(loot::Language::any).Str(); + + cPluginMessages[i].type = message.Type(); + cPluginMessages[i].message = pluginMessageStrings[i].c_str(); + + ++i; + } + } + + void addBashTagsToMap(std::set names) { + for (const auto& name : names) { + // Try adding the Bash Tag to the map assuming it's not already in + // there, then use the UID in the returned value, as that will be + // equal to the value in the map, even if the Bash Tag was already + // present. + unsigned int uid = bashTagMap.size(); + // If the tag already exists in the map, do + auto it = bashTagMap.emplace(name, uid).first; + if (it->second == cBashTagMap.size()) + cBashTagMap.push_back(it->first.c_str()); + else + cBashTagMap.at(it->second) = it->first.c_str(); + } + } + + void clearBashTagMap() { + bashTagMap.clear(); + cBashTagMap.clear(); + } + + void clearArrays() { + pluginNames.clear(); + cPluginNames.clear(); + + addedTagIds.clear(); + removedTagIds.clear(); + + cPluginMessages.clear(); + pluginMessageStrings.clear(); + } +private: + std::string revisionId; + std::string revisionDate; + + std::vector pluginNames; + std::vector cPluginNames; + + // For the Bash Tag map, a string is mapped to a UID that is also the + // index of the vector where the C string can be found. std::unordered_map bashTagMap; + std::vector cBashTagMap; - char ** extTagMap; + std::vector addedTagIds; + std::vector removedTagIds; - char ** extStringArray; - size_t extStringArraySize; - - char * extRevisionID; - char * extRevisionDate; - - unsigned int * extAddedTagIds; - unsigned int * extRemovedTagIds; - - loot_message * extMessageArray; - size_t extMessageArraySize; + std::vector cPluginMessages; + std::vector pluginMessageStrings; }; -char * extMessageStr = nullptr; - -// std::string to null-terminated char string converter. -char * ToNewCString(std::string str) { - char * p = new char[str.length() + 1]; - return strcpy(p, str.c_str()); -} +std::string extMessageStr; unsigned int c_error(const loot::error& e) { - delete[] extMessageStr; - try { - extMessageStr = new char[strlen(e.what()) + 1]; - strcpy(extMessageStr, e.what()); - } - catch (std::bad_alloc& /*e*/) { - extMessageStr = nullptr; - } + extMessageStr = e.what(); return e.code(); } @@ -185,15 +264,14 @@ LOOT_API unsigned int loot_get_error_message(const char ** const message) { if (message == nullptr) return c_error(loot_error_invalid_args, "Null message pointer passed."); - *message = extMessageStr; + *message = extMessageStr.c_str(); return loot_ok; } // Frees memory allocated to error string. LOOT_API void loot_cleanup() { - delete[] extMessageStr; - extMessageStr = nullptr; + extMessageStr.clear(); } ////////////////////////////// @@ -345,26 +423,8 @@ LOOT_API unsigned int loot_load_lists(loot_db db, const char * const masterlistP } //Also free memory. - db->bashTagMap.clear(); - delete[] db->extAddedTagIds; - delete[] db->extRemovedTagIds; - - if (db->extTagMap != nullptr) { - for (size_t i = 0; i < db->bashTagMap.size(); i++) - delete[] db->extTagMap[i]; //Gotta clear those allocated strings. - delete[] db->extTagMap; - } - - if (db->extMessageArray != nullptr) { - for (size_t i = 0; i < db->extMessageArraySize; i++) - delete[] db->extMessageArray[i].message; //Gotta clear those allocated strings. - delete[] db->extMessageArray; - } - - db->extAddedTagIds = nullptr; - db->extRemovedTagIds = nullptr; - db->extTagMap = nullptr; - db->extMessageArray = nullptr; + db->clearBashTagMap(); + db->clearArrays(); db->GetMasterlist() = temp; db->rawMetadata = temp; @@ -425,15 +485,6 @@ LOOT_API unsigned int loot_sort_plugins(loot_db db, if (db == nullptr || sortedPlugins == nullptr || numPlugins == nullptr) return c_error(loot_error_invalid_args, "Null pointer passed."); - //Clear existing array allocation. - if (db->extStringArray != nullptr) { - for (size_t i = 0; i < db->extStringArraySize; ++i) { - delete[] db->extStringArray[i]; - } - delete[] db->extStringArray; - db->extStringArray = nullptr; - } - //Initialise output. *numPlugins = 0; *sortedPlugins = nullptr; @@ -444,16 +495,8 @@ LOOT_API unsigned int loot_sort_plugins(loot_db db, //Sort plugins into their load order. loot::PluginSorter sorter; - std::list plugins = sorter.Sort(*db, loot_lang_any, [](const std::string& message) {}); - db->extStringArraySize = plugins.size(); - db->extStringArray = new char*[db->extStringArraySize]; - - size_t i = 0; - for (const auto &plugin : plugins) { - db->extStringArray[i] = ToNewCString(plugin.Name()); - ++i; - } + db->setPluginNames(sorter.Sort(*db, loot_lang_any, [](const std::string& message) {})); } catch (loot::error &e) { return c_error(e); @@ -462,8 +505,8 @@ LOOT_API unsigned int loot_sort_plugins(loot_db db, return c_error(loot_error_no_mem, e.what()); } - *numPlugins = db->extStringArraySize; - *sortedPlugins = db->extStringArray; + *numPlugins = db->getPluginNames().size(); + *sortedPlugins = const_cast(&db->getPluginNames()[0]); return loot_ok; } @@ -532,8 +575,8 @@ LOOT_API unsigned int loot_get_masterlist_revision(loot_db db, edited = true; } - db->extRevisionID = ToNewCString(id); - db->extRevisionDate = ToNewCString(date); + db->setRevisionIdString(id); + db->setRevisionDateString(date); } catch (loot::error &e) { if (e.code() == loot_ok) @@ -545,8 +588,8 @@ LOOT_API unsigned int loot_get_masterlist_revision(loot_db db, return c_error(loot_error_no_mem, e.what()); } - *revisionID = db->extRevisionID; - *revisionDate = db->extRevisionDate; + *revisionID = const_cast(db->getRevisionIdString()); + *revisionDate = const_cast(db->getRevisionDateString()); *isModified = edited; return loot_ok; @@ -564,13 +607,7 @@ LOOT_API unsigned int loot_get_tag_map(loot_db db, char *** const tagMap, size_t return c_error(loot_error_invalid_args, "Null pointer passed."); //Clear existing array allocation. - if (db->extTagMap != nullptr) { - for (size_t i = 0, max = db->bashTagMap.size(); i < max; ++i) { - delete[] db->extTagMap[i]; - } - delete[] db->extTagMap; - db->extTagMap = nullptr; - } + db->clearBashTagMap(); //Initialise output. *tagMap = nullptr; @@ -593,27 +630,14 @@ LOOT_API unsigned int loot_get_tag_map(loot_db db, char *** const tagMap, size_t return loot_ok; try { - db->extTagMap = new char*[allTags.size()]; + db->addBashTagsToMap(allTags); } catch (std::bad_alloc& e) { return c_error(loot_error_no_mem, e.what()); } - try { - unsigned int UID = 0; - for (const auto &tag : allTags) { - db->bashTagMap.insert(std::pair(tag, UID)); - //Also allocate memory. - db->extTagMap[UID] = ToNewCString(tag); - UID++; - } - } - catch (std::bad_alloc& e) { - return c_error(loot_error_no_mem, e.what()); - } - - *tagMap = db->extTagMap; - *numTags = allTags.size(); + *tagMap = const_cast(&db->getBashTagMap()[0]); + *numTags = db->getBashTagMap().size(); return loot_ok; } @@ -634,16 +658,10 @@ LOOT_API unsigned int loot_get_plugin_tags(loot_db db, const char * const plugin if (db == nullptr || plugin == nullptr || tagIds_added == nullptr || numTags_added == nullptr || tagIds_removed == nullptr || numTags_removed == nullptr || userlistModified == nullptr) return c_error(loot_error_invalid_args, "Null pointer passed."); - if (db->bashTagMap.empty()) { + if (db->getBashTagMap().empty()) { return c_error(loot_error_no_tag_map, "No Bash Tag map has been previously generated."); } - //Clear existing array allocations. - delete[] db->extAddedTagIds; - delete[] db->extRemovedTagIds; - db->extAddedTagIds = nullptr; - db->extRemovedTagIds = nullptr; - //Initialise output. *tagIds_added = nullptr; *tagIds_removed = nullptr; @@ -670,42 +688,19 @@ LOOT_API unsigned int loot_get_plugin_tags(loot_db db, const char * const plugin tagsRemoved.insert(tag.Name()); } - std::vector tagsAddedIDs, tagsRemovedIDs; - for (const auto &tagName : tagsAdded) { - const auto mapIter(db->bashTagMap.find(tagName)); - if (mapIter != db->bashTagMap.end()) - tagsAddedIDs.push_back(mapIter->second); - } - for (const auto &tagName : tagsRemoved) { - const auto mapIter(db->bashTagMap.find(tagName)); - if (mapIter != db->bashTagMap.end()) - tagsRemovedIDs.push_back(mapIter->second); - } - - //Allocate memory. - size_t numAdded = tagsAddedIDs.size(); - size_t numRemoved = tagsRemovedIDs.size(); try { - if (numAdded != 0) { - db->extAddedTagIds = new uint32_t[numAdded]; - for (size_t i = 0; i < numAdded; i++) - db->extAddedTagIds[i] = tagsAddedIDs[i]; - } - if (numRemoved != 0) { - db->extRemovedTagIds = new uint32_t[numRemoved]; - for (size_t i = 0; i < numRemoved; i++) - db->extRemovedTagIds[i] = tagsRemovedIDs[i]; - } + db->setAddedTags(tagsAdded); + db->setRemovedTags(tagsRemoved); } - catch (std::bad_alloc& e) { - return c_error(loot_error_no_mem, e.what()); + catch (loot::error& e) { + return c_error(e); } //Set outputs. - *tagIds_added = db->extAddedTagIds; - *tagIds_removed = db->extRemovedTagIds; - *numTags_added = numAdded; - *numTags_removed = numRemoved; + *tagIds_added = reinterpret_cast(&db->getAddedTagIds()[0]); + *tagIds_removed = reinterpret_cast(&db->getRemovedTagIds()[0]); + *numTags_added = db->getAddedTagIds().size(); + *numTags_removed = db->getRemovedTagIds().size(); return loot_ok; } @@ -719,16 +714,6 @@ LOOT_API unsigned int loot_get_plugin_messages(loot_db db, const char * const pl if (db == nullptr || plugin == nullptr || messages == nullptr || numMessages == nullptr) return c_error(loot_error_invalid_args, "Null pointer passed."); - //Clear existing array allocation. - if (db->extMessageArray != nullptr) { - for (size_t i = 0; i < db->extMessageArraySize; ++i) { - delete[] db->extMessageArray[i].message; - } - delete[] db->extMessageArray; - db->extMessageArray = nullptr; - db->extMessageArraySize = 0; - } - //Initialise output. *messages = nullptr; *numMessages = 0; @@ -740,24 +725,13 @@ LOOT_API unsigned int loot_get_plugin_messages(loot_db db, const char * const pl std::list temp(p.Messages()); pluginMessages.insert(pluginMessages.end(), temp.begin(), temp.end()); - if (!pluginMessages.empty()) { - db->extMessageArraySize = pluginMessages.size(); - try { - db->extMessageArray = new loot_message[db->extMessageArraySize]; - int i = 0; - for (const auto &message : pluginMessages) { - db->extMessageArray[i].type = message.Type(); - db->extMessageArray[i].message = ToNewCString(message.ChooseContent(loot::Language::any).Str()); - ++i; - } - } - catch (std::bad_alloc& e) { - return c_error(loot_error_no_mem, e.what()); - } - } + if (pluginMessages.empty()) + return loot_ok; - *messages = db->extMessageArray; - *numMessages = db->extMessageArraySize; + db->setPluginMessages(pluginMessages); + + *messages = const_cast(&db->getPluginMessages()[0]); + *numMessages = db->getPluginMessages().size(); return loot_ok; }