From a9dae134d0d577a928521fcc8631536f34b2cf40 Mon Sep 17 00:00:00 2001 From: Anna Schumaker Date: Tue, 25 Mar 2014 10:57:09 -0400 Subject: [PATCH] database: Store pointers in the database Inserting into a vector can sometimes cause the entire vector to reallocate itself. The insert() function returns a pointer to the caller, so this reallocation could invalidate the returned pointer. This is not what we want. Instead, store pointers to the data in the vector. C++ provides a default copy constructor that can be used to allocate a new item before inserting. By doing it this way callers won't have to allocate memory themselves. In addition, I will no longer need to keep a valid bit since we can simply check for a NULL entry in the database. Signed-off-by: Anna Schumaker --- DESIGN | 40 +++++++++++++-------------- include/database.h | 8 +++--- include/database.hpp | 56 +++++++++++++++++++++----------------- lib/database.cpp | 4 ++- lib/library.cpp | 33 ++++++++++------------ lib/tags.cpp | 4 +-- tests/db_entry | 4 +-- tests/src/database.cpp | 62 ++++++++++++++++++++++++++++++++++-------- tests/src/db_entry.cpp | 2 +- 9 files changed, 127 insertions(+), 86 deletions(-) diff --git a/DESIGN b/DESIGN index 25bec4d7..a006e590 100644 --- a/DESIGN +++ b/DESIGN @@ -199,7 +199,6 @@ Database Entry: - DatabaseEntry: class DatabaseEntry { public: - bool valid; unsigned int id; DatabaseEntry(); @@ -212,7 +211,7 @@ Database Entry: - API: DatabaseEntry :: DatabaseEntry(): - Set valid = false and id = 0. + Set id = 0. const std::string DatabaseEntry :: primary_key(); This function should return a unique string representing this @@ -251,12 +250,6 @@ Database: uniqueness. This key is used when inserting a new value into the Database, and will not be updated after. -- Valid bit: - The "valid" bit of a DatabaseEntry is completely managed by the entry's - Database container. It will be set to true when an entry is inserted - and false when deleted. The Database is also in charge of writing the - valid bit to file. - - Id: The "id" field of a DatabaseEntry is also managed by the entry's Database container It will be set to the entry's ID when the entry is @@ -266,17 +259,18 @@ Database: template class Database { private: - std::vector _db; + std::vector _db; std::map _keys; unsigned int _size; /* Number of valid rows */ bool _autosave; File _file; public: - typedef std::vector::iterator iterator; - typedef std::vector::const_iterator const_iterator; + typedef std::vector::iterator iterator; + typedef std::vector::const_iterator const_iterator; Database(std::string, bool); + ~Database(); void save(); void load(); @@ -305,28 +299,32 @@ Database: data on disk. If the file already exists, read the data into the backing vector. + Database :: ~Database(); + Delete all entries remaining in the database. + void Database :: save(); Save the database to disk. void Database :: load(); Load the database from disk. - T *Database :: insert(T &item); + T *Database :: insert(T item); Look up the item in the _keys map. If we find an item with the same key: - - Return an iterator to the found item. + - Return a pointer to the found item. Otherwise: + - Use new to allocate memory for a new item. - Add the new item to the end of the _db. - Add the new item to _keys. - - Set item.valid = true. - - Set item.id to the index of the new item. + - Set new item.id to the index of the new item. _ Increment _size. - If autosave == true: save(). - - Return an iterator to the new item. + - Return a pointer to the new item. - unsigned int Database :: remove(); + unsigned int Database :: remove(unsigned int id); - Remove the item from the _keys map. - - Set item.valid = false. + - Delete _db[id] + - Set _db[id] = NULL - If autosave == true: save(). - Decrement _size. @@ -351,10 +349,10 @@ Database: if there are no valid entries left. T *Database :: at(unsigned int i); - If _db[i].valid == true: - Return a pointer to _db[i]; + If i is beyond the end of the container: + Return NULL Otherwise: - Return NULL; + Return _db[i]; T *Database :: find(const std::string &key); If key is in the _keys map: diff --git a/include/database.h b/include/database.h index 845f4967..8c9e5493 100644 --- a/include/database.h +++ b/include/database.h @@ -13,10 +13,10 @@ class DatabaseEntry { public: - bool valid; unsigned int id; DatabaseEntry(); + virtual ~DatabaseEntry() = 0; virtual const std::string primary_key() = 0; virtual void write(File &) = 0; virtual void read(File &) = 0; @@ -27,15 +27,15 @@ public: template class Database { private: - std::vector _db; + std::vector _db; std::map _keys; unsigned int _size; bool _autosave; File _file; public: - typedef typename std::vector::iterator iterator; - typedef typename std::vector::const_iterator const_iterator; + typedef typename std::vector::iterator iterator; + typedef typename std::vector::const_iterator const_iterator; Database(std::string, bool); ~Database(); diff --git a/include/database.hpp b/include/database.hpp index bc486f14..89163090 100644 --- a/include/database.hpp +++ b/include/database.hpp @@ -18,6 +18,9 @@ Database :: Database(std::string filepath, bool autosave) template Database :: ~Database() { + iterator it; + for (it = begin(); it != end(); it = next(it)) + delete (*it); } template @@ -26,12 +29,13 @@ void Database :: save() if (_file.open(OPEN_WRITE) == false) return; - _file << _db.size() << std::endl; + _file << actual_size() << std::endl; for (unsigned int i = 0; i < _db.size(); i++) { - _file << _db[i].valid; - if (_db[i].valid == true) { - _file << " "; - _db[i].write(_file); + if (_db[i] == NULL) + _file << false; + else { + _file << true << " "; + _db[i]->write(_file); } _file << std::endl; } @@ -50,6 +54,7 @@ template void Database :: load() { unsigned int db_size; + bool valid; if (_file.exists() == false) return; @@ -59,11 +64,14 @@ void Database :: load() _file >> db_size; _db.resize(db_size); for (unsigned int i = 0; i < db_size; i++) { - _file >> _db[i].valid; - if (_db[i].valid == true) { - _db[i].read(_file); - _db[i].id = i; - _keys.insert(std::pair(_db[i].primary_key(), i)); + _file >> valid; + if (valid == false) + _db[i] = NULL; + else { + _db[i] = new T; + _db[i]->read(_file); + _db[i]->id = i; + _keys[_db[i]->primary_key()] = i; _size++; } } @@ -79,14 +87,14 @@ T *Database :: insert(T val) if (t != NULL) return t; - iterator it = _db.insert(_db.end(), val); - it->valid = true; - it->id = it - _db.begin(); + t = new T(val); + _db.push_back(t); + t->id = actual_size() - 1; - _keys[it->primary_key()] = it->id; + _keys[t->primary_key()] = t->id; _size++; autosave(); - return &_db[it->id]; + return t; } template @@ -94,10 +102,11 @@ void Database :: remove(unsigned int id) { if (id >= actual_size()) return; - if (_db[id].valid == false) + if (_db[id] == NULL) return; - _keys.erase(_db[id].primary_key()); - _db[id].valid = false; + _keys.erase(_db[id]->primary_key()); + delete _db[id]; + _db[id] = NULL; _size--; autosave(); } @@ -121,7 +130,7 @@ typename Database::iterator Database :: begin() return end(); iterator it = _db.begin(); - if ( (*it).valid == true ) + if ( (*it) != NULL ) return it; return next(it); } @@ -137,7 +146,7 @@ typename Database::iterator Database :: next(iterator &it) { do { it++; - } while ((it != end()) && ((*it).valid == false)); + } while ((it != end()) && (*it) == NULL); return it; } @@ -146,10 +155,7 @@ T *Database :: at(unsigned int id) { if (id >= actual_size()) return NULL; - - if (_db[id].valid == false) - return NULL; - return &_db[id]; + return _db[id]; } template @@ -159,6 +165,6 @@ T *Database :: find(const std::string &key) it = _keys.find(key); if (it == _keys.end()) return NULL; - return &_db[it->second]; + return _db[it->second]; } #endif /* OCARINA_DATABASE_HPP */ diff --git a/lib/database.cpp b/lib/database.cpp index a68af4ae..f520cf56 100644 --- a/lib/database.cpp +++ b/lib/database.cpp @@ -5,6 +5,8 @@ DatabaseEntry :: DatabaseEntry() - : valid(false), id(0) + : id(0) { } + +DatabaseEntry :: ~DatabaseEntry() {} diff --git a/lib/library.cpp b/lib/library.cpp index 4f4e9e43..58b64010 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -282,9 +282,6 @@ static void read_tags(unsigned int lib_id, const std :: string &path) track_id = track_db.insert(library :: Track(tag, audio, lib_id, artist_id, album_id, genre_id, path))->id; - if (track_db.at(track_id)->valid == false) - return; - library_db.at(lib_id)->size++; filter::add(artist_db.at(artist_id)->name, track_id); @@ -356,10 +353,10 @@ static void do_validate_library(unsigned int &lib_id) Database::iterator it; for (it = track_db.begin(); it != track_db.end(); it = track_db.next(it)) { - if ((*it).library_id != lib_id) + if ((*it)->library_id != lib_id) continue; - path = it->primary_key(); + path = (*it)->primary_key(); if (g_file_test(path.c_str(), G_FILE_TEST_EXISTS) == false) { dprint("Removing file: %s\n", path.c_str()); track_db.remove(it - track_db.begin()); @@ -469,17 +466,17 @@ void library :: init() Database::iterator it; for (it = track_db.begin(); it != track_db.end(); it = track_db.next(it)) { - filter::add(artist_db.at((*it).artist_id)->name, it->id); - filter::add(album_db.at((*it).album_id)->name, it->id); - filter::add((*it).title, it->id); + filter::add(artist_db.at((*it)->artist_id)->name, (*it)->id); + filter::add(album_db.at((*it)->album_id)->name, (*it)->id); + filter::add((*it)->title, (*it)->id); - if (library_db.at((*it).library_id)->enabled) - get_callbacks()->on_library_track_add(it->id); + if (library_db.at((*it)->library_id)->enabled) + get_callbacks()->on_library_track_add((*it)->id); } Database::iterator l_it; for (l_it = library_db.begin(); l_it != library_db.end(); l_it = library_db.next(l_it)) - get_callbacks()->on_library_add(l_it->id, &(*library_db.at(l_it->id))); + get_callbacks()->on_library_add((*l_it)->id, &(*library_db.at((*l_it)->id))); } void library :: add_path(const std::string &dir) @@ -502,9 +499,9 @@ void library :: del_path(unsigned int id) Database::iterator it; for (it = track_db.begin(); it != track_db.end(); it = track_db.next(it)) { - if ((*it).library_id == id) { - get_callbacks()->on_library_track_del(it->id); - track_db.remove(it->id); + if ((*it)->library_id == id) { + get_callbacks()->on_library_track_del((*it)->id); + track_db.remove((*it)->id); } } @@ -518,7 +515,7 @@ void library :: update_path(unsigned int id) { if (id > library_db.size()) return; - if (library_db.at(id)->valid == false) + if (library_db.at(id) == NULL) return; do_update_library(id); } @@ -540,7 +537,7 @@ void library :: set_enabled(unsigned int id, bool enabled) library_db.save(); for (it = track_db.begin(); it != track_db.end(); it = track_db.next(it)) { - if ((*it).library_id == id) { + if ((*it)->library_id == id) { t = it - track_db.begin(); if (enabled) get_callbacks()->on_library_track_add(t); @@ -556,7 +553,7 @@ void library :: lookup(unsigned int id, library :: Song *song) throw -E_EXIST; song->track = &(*track_db.at(id)); - if (song->track->valid == false) + if (song->track == NULL) throw -E_EXIST; song->track_id = id; @@ -570,7 +567,7 @@ library :: Library *library :: lookup_path(unsigned int id) { if (id >= library_db.actual_size()) throw -E_EXIST; - if (library_db.at(id)->valid == false) + if (library_db.at(id) == NULL) throw -E_EXIST; return &(*library_db.at(id)); } diff --git a/lib/tags.cpp b/lib/tags.cpp index 17a25aa5..db898f36 100644 --- a/lib/tags.cpp +++ b/lib/tags.cpp @@ -221,8 +221,8 @@ void tagdb :: remove_library(unsigned int library_id) { Database::iterator it; for (it = track_db.begin(); it != track_db.end(); it++) { - if (it->library->id == library_id) - track_db.remove(it->id); + if ((*it)->library->id == library_id) + track_db.remove((*it)->id); } library_db.remove(library_id); } diff --git a/tests/db_entry b/tests/db_entry index 33f97f49..3517caeb 100755 --- a/tests/db_entry +++ b/tests/db_entry @@ -10,7 +10,7 @@ function test_entry function test_print { - test_entry $1 $2 "Value: $1 Key: $2 Valid: 0 Id: 0" + test_entry $1 $2 "Value: $1 Key: $2 Id: 0" } function test_primary_key @@ -30,7 +30,7 @@ function test_read rm $DATA_DIR/db_entry.txt 2>/dev/null || true echo 0 > $DATA_DIR/db_entry.txt echo $1 $2 >> $DATA_DIR/db_entry.txt - test_entry "-r $1" $2 "Value: $1 Key: $2 Valid: 0 Id: 0" + test_entry "-r $1" $2 "Value: $1 Key: $2 Id: 0" } diff --git a/tests/src/database.cpp b/tests/src/database.cpp index 365b9c79..34b959d2 100644 --- a/tests/src/database.cpp +++ b/tests/src/database.cpp @@ -84,9 +84,24 @@ int main(int argc, char **argv) /** * 1: Test insertion */ + IntEntry *one, *three; for (unsigned int i = 0; i < n; i++) { - if (db.insert(IntEntry(i))->id != i) + IntEntry *it = db.insert(IntEntry(i)); + if (it == NULL) test_results(false, __LINE__); + if (it->id != i) + test_results(false, __LINE__); + if (it->val != i) + test_results(false, __LINE__); + + /* + * Pointers should still be valid after more insertions. + * These will be checked later + */ + if (i == 1) + one = it; + if (i == 3) + three = it; } test_results(true, __LINE__); size += n; @@ -103,7 +118,12 @@ int main(int argc, char **argv) * 3: Test inserting ... again. */ for (unsigned int i = 0; i < n; i++) { - if (db.insert(IntEntry(i))->id != i) + IntEntry *it = db.insert(IntEntry(i)); + if (it->id != i) + test_results(false, __LINE__); + if (i == 1 && it != one) + test_results(false, __LINE__); + if (i == 3 && it != three) test_results(false, __LINE__); } test_results(true, __LINE__); @@ -132,7 +152,7 @@ int main(int argc, char **argv) /** * 6: Test that removing again doesn't change anything */ - for (unsigned int i = 0; i < n; i+=2) + for (unsigned int i = 0; i < n + 10; i+=2) db.remove(i); test_size(db, size, actual, __LINE__); @@ -143,9 +163,11 @@ int main(int argc, char **argv) Database::iterator it; unsigned int index = 1; for (it = db.begin(); it != db.end(); it = db.next(it)) { - if ((*it).val != index) + if ((*it) == NULL) test_results(false, __LINE__); - if ((*it).id != index) + if ((*it)->val != index) + test_results(false, __LINE__); + if ((*it)->id != index) test_results(false, __LINE__); index+=2; }; @@ -157,9 +179,23 @@ int main(int argc, char **argv) */ for (unsigned int i = 0; i < n + 10; i++) { IntEntry *it = db.at(i); - if (((i % 2) == 0) && (it != NULL)) + if (i >= n) { + if (it != NULL) + test_results(false, __LINE__); + continue; + } else if (i % 2 == 0) { + if (it != NULL) + test_results(false, __LINE__); + } else if (it == NULL) test_results(false, __LINE__); - if ((i >= n) && (it != NULL)) + + /* + * Remember those pointers we stored earlier? + * The should still be valid now. + */ + if (i == 1 && it != one) + test_results(false, __LINE__); + if (i == 3 && it != three) test_results(false, __LINE__); } test_results(true, __LINE__); @@ -169,7 +205,7 @@ int main(int argc, char **argv) * 9. Test inserting once again */ for (unsigned int i = 0; i < n; i++) { - index = db.insert(i)->id; + index = db.insert(IntEntry(i))->id; if ((i % 2) == 0) { size++; actual++; @@ -210,12 +246,14 @@ int main(int argc, char **argv) */ Database::iterator it2 = db2.begin(); for (it = db.begin(); it != db.end(); it++) { - if (it->valid != it2->valid) + if ((*it) == NULL && (*it2) != NULL) test_results(false, __LINE__); - if (it->valid == true) { - if (it->val != it2->val) + if ((*it) != NULL && (*it2) == NULL) + test_results(false, __LINE__); + if ((*it) != NULL) { + if ((*it)->val != (*it2)->val) test_results(false, __LINE__); - if (it->id != it2->id) + if ((*it)->id != (*it2)->id) test_results(false, __LINE__); } it2++; diff --git a/tests/src/db_entry.cpp b/tests/src/db_entry.cpp index f5aac956..fca5228c 100644 --- a/tests/src/db_entry.cpp +++ b/tests/src/db_entry.cpp @@ -48,7 +48,7 @@ void IntEntry :: read(File &f) void IntEntry :: print() { - :: print("Value: %u Key: %s Valid: %d Id: %u\n", val, key.c_str(), valid, id); + :: print("Value: %u Key: %s Id: %u\n", val, key.c_str(), id); }