Debloat and slightly speed up the MRU code

libaegisub/common/mru.cpp

@@ -22,35 +22,63 @@
 #include "libaegisub/option.h"
 #include "libaegisub/option_value.h"
 
+namespace {
+const char *mru_names[] = {
+	"Audio",
+	"Find",
+	"Keyframes",
+	"Replace",
+	"Subtitle",
+	"Timecodes",
+	"Video",
+};
+
+const char *option_names[] = {
+	"Limits/MRU",
+	"Limits/Find Replace",
+	"Limits/MRU",
+	"Limits/Find Replace",
+	"Limits/MRU",
+	"Limits/MRU",
+	"Limits/MRU",
+};
+
+int mru_index(const char *key) {
+	int i;
+	switch (*key) {
+	case 'A': i = 0; break;
+	case 'F': i = 1; break;
+	case 'K': i = 2; break;
+	case 'R': i = 3; break;
+	case 'S': i = 4; break;
+	case 'T': i = 5; break;
+	case 'V': i = 6; break;
+	default: return -1;
+	}
+	return strcmp(key, mru_names[i]) == 0 ? i : -1;
+}
+}
+
 namespace agi {
 MRUManager::MRUManager(agi::fs::path const& config, std::pair<const char *, size_t> default_config, agi::Options *options)
 : config_name(config)
 , options(options)
-, option_names({
-	{"Audio", "Limits/MRU"},
-	{"Find", "Limits/Find Replace"},
-	{"Keyframes", "Limits/MRU"},
-	{"Replace", "Limits/Find Replace"},
-	{"Subtitle", "Limits/MRU"},
-	{"Timecodes", "Limits/MRU"},
-	{"Video", "Limits/MRU"},
-})
 {
 	LOG_D("agi/mru") << "Loading MRU List";
 
 	auto root = json_util::file(config, default_config);
 	for (auto const& it : static_cast<json::Object const&>(root))
-		Load(it.first, it.second);
+		Load(it.first.c_str(), it.second);
 }
 
-MRUManager::MRUListMap &MRUManager::Find(std::string const& key) {
+MRUManager::MRUListMap &MRUManager::Find(const char *key) {
-	auto index = mru.find(key);
+	auto index = mru_index(key);
-	if (index == mru.end())
+	if (index == -1)
-		throw MRUErrorInvalidKey("Invalid key value");
+		throw MRUError("Invalid key value");
-	return index->second;
+	return mru[index];
 }
 
-void MRUManager::Add(std::string const& key, agi::fs::path const& entry) {
+void MRUManager::Add(const char *key, agi::fs::path const& entry) {
 	MRUListMap &map = Find(key);
 	auto it = find(begin(map), end(map), entry);
 	if (it == begin(map) && it != end(map))
@@ -65,20 +93,20 @@ void MRUManager::Add(std::string const& key, agi::fs::path const& entry) {
 	Flush();
 }
 
-void MRUManager::Remove(std::string const& key, agi::fs::path const& entry) {
+void MRUManager::Remove(const char *key, agi::fs::path const& entry) {
 	auto& map = Find(key);
 	map.erase(remove(begin(map), end(map), entry), end(map));
 	Flush();
 }
 
-const MRUManager::MRUListMap* MRUManager::Get(std::string const& key) {
+const MRUManager::MRUListMap* MRUManager::Get(const char *key) {
 	return &Find(key);
 }
 
-agi::fs::path const& MRUManager::GetEntry(std::string const& key, const size_t entry) {
+agi::fs::path const& MRUManager::GetEntry(const char *key, const size_t entry) {
 	const auto map = Get(key);
 	if (entry >= map->size())
-		throw MRUErrorIndexOutOfRange("Requested element index is out of range.");
+		throw MRUError("Requested element index is out of range.");
 
 	return *next(map->begin(), entry);
 }
@@ -86,34 +114,38 @@ agi::fs::path const& MRUManager::GetEntry(std::string const& key, const size_t e
 void MRUManager::Flush() {
 	json::Object out;
 
-	for (auto const& mru_map : mru) {
+	for (size_t i = 0; i < mru.size(); ++i) {
-		json::Array &array = out[mru_map.first];
+		json::Array &array = out[mru_names[i]];
-		for (auto const& p : mru_map.second)
+		for (auto const& p : mru[i])
 			array.push_back(p.string());
 	}
 
 	agi::JsonWriter::Write(out, io::Save(config_name).Get());
 }
 
-void MRUManager::Prune(std::string const& key, MRUListMap& map) const {
+void MRUManager::Prune(const char *key, MRUListMap& map) const {
 	size_t limit = 16u;
 	if (options) {
-		auto it = option_names.find(key);
+		int idx = mru_index(key);
-		if (it != option_names.end())
+		if (idx != -1)
-			limit = (size_t)options->Get(it->second)->GetInt();
+			limit = (size_t)options->Get(option_names[idx])->GetInt();
 	}
 	map.resize(std::min(limit, map.size()));
 }
 
-void MRUManager::Load(std::string const& key, const json::Array& array) {
+void MRUManager::Load(const char *key, const json::Array& array) {
+	int idx = mru_index(key);
+	if (idx == -1) return;
+
 	try {
-		transform(begin(array), end(array),
+		mru[idx].reserve(array.size());
-			back_inserter(mru[key]), [](std::string const& s) { return s; });
+		for (std::string const& str : array)
+			mru[idx].push_back(str);
 	}
 	catch (json::Exception const&) {
 		// Out of date MRU file; just discard the data and skip it
 	}
-	Prune(key, mru[key]);
+	Prune(key, mru[idx]);
 }
 
 }

libaegisub/include/libaegisub/mru.h

@@ -12,8 +12,8 @@
 // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
+#include <array>
 #include <boost/filesystem/path.hpp>
-#include <map>
 #include <vector>
 
 #include <libaegisub/exception.h>
@@ -28,8 +28,6 @@ namespace agi {
 class Options;
 
 DEFINE_EXCEPTION(MRUError, Exception);
-DEFINE_EXCEPTION(MRUErrorInvalidKey, MRUError);
-DEFINE_EXCEPTION(MRUErrorIndexOutOfRange, MRUError);
 
 /// @class MRUManager
 /// @brief Most Recently Used (MRU) list handling
@@ -54,25 +52,25 @@ public:
 	/// @brief Add entry to the list.
 	/// @param key List name
 	/// @param entry Entry to add
-	/// @exception MRUErrorInvalidKey thrown when an invalid key is used.
+	/// @exception MRUError thrown when an invalid key is used.
-	void Add(std::string const& key, agi::fs::path const& entry);
+	void Add(const char *key, agi::fs::path const& entry);
 
 	/// @brief Remove entry from the list.
 	/// @param key List name
 	/// @param entry Entry to add
-	/// @exception MRUErrorInvalidKey thrown when an invalid key is used.
+	/// @exception MRUError thrown when an invalid key is used.
-	void Remove(std::string const& key, agi::fs::path const& entry);
+	void Remove(const char *key, agi::fs::path const& entry);
 
 	/// @brief Return list
 	/// @param key List name
-	/// @exception MRUErrorInvalidKey thrown when an invalid key is used.
+	/// @exception MRUError thrown when an invalid key is used.
-	const MRUListMap* Get(std::string const& key);
+	const MRUListMap* Get(const char *key);
 
 	/// @brief Return A single entry in a list.
 	/// @param key List name
 	/// @param entry 0-base position of entry
-	/// @exception MRUErrorInvalidKey thrown when an invalid key is used.
+	/// @exception MRUError thrown when an invalid key is used.
-	agi::fs::path const& GetEntry(std::string const& key, const size_t entry);
+	agi::fs::path const& GetEntry(const char *key, const size_t entry);
 
 	/// Write MRU lists to disk.
 	void Flush();
@@ -84,25 +82,17 @@ private:
 	/// User preferences object for maximum number of items to list
 	agi::Options *const options;
 
-	/// @brief Map for MRUListMap values.
-	/// @param std::string Name
-	/// @param MRUListMap instance.
-	using MRUMap = std::map<std::string, MRUListMap>;
-
 	/// Internal MRUMap values.
-	MRUMap mru;
+	std::array<MRUListMap, 7> mru;
-
-	/// Map from MRU name to option name
-	const std::map<std::string, std::string> option_names;
 
 	/// @brief Load MRU Lists.
 	/// @param key List name.
 	/// @param array json::Array of values.
-	void Load(std::string const& key, ::json::Array const& array);
+	void Load(const char *key, ::json::Array const& array);
 	/// @brief Prune MRUListMap to the desired length.
 	/// This uses the user-set values for MRU list length.
-	void Prune(std::string const& key, MRUListMap& map) const;
+	void Prune(const char *key, MRUListMap& map) const;
-	MRUListMap &Find(std::string const& key);
+	MRUListMap &Find(const char *key);
 };
 
 } // namespace agi

src/compat.cpp

@@ -4,7 +4,7 @@
 
 #include <algorithm>
 
-wxArrayString lagi_MRU_wxAS(std::string const& list) {
+wxArrayString lagi_MRU_wxAS(const char *list) {
 	auto const& vec = *config::mru->Get(list);
 	wxArrayString ret;
 	ret.reserve(vec.size());

src/compat.h

@@ -16,4 +16,4 @@ wxArrayString to_wx(std::vector<std::string> const& vec);
 inline agi::Color from_wx(wxColour color) { return agi::Color(color.Red(), color.Green(), color.Blue(), 255 - color.Alpha()); }
 inline std::string from_wx(wxString const& str) { return std::string(str.utf8_str()); }
 
-wxArrayString lagi_MRU_wxAS(std::string const& list);
+wxArrayString lagi_MRU_wxAS(const char *list);

src/menu.cpp

@@ -84,7 +84,7 @@ public:
 	}
 
 	void Update() {
-		const auto mru = config::mru->Get(type);
+		const auto mru = config::mru->Get(type.c_str());
 
 		Resize(mru->size());
 

tests/setup.bat

@@ -20,9 +20,9 @@ icacls data\dir_access_denied /deny %USERNAME%:F
 mkdir data\dir_read_only
 icacls data\dir_read_only /deny %USERNAME%:W
 
-echo {"Valid" : ["Entry One", "Entry Two"]} > data/mru_ok.json
+echo {"Video" : ["Entry One", "Entry Two"]} > data/mru_ok.json
 
-echo {"Invalid" : [1, 3]} > data/mru_invalid.json
+echo {"Video" : [1, 3]} > data/mru_invalid.json
 
 echo '1234567890' > data\ten_bytes
 echo '' > data\touch_mod_time

tests/setup.sh

@@ -23,9 +23,9 @@ chmod 000 data/dir_access_denied
 mkdir data/dir_read_only
 chmod 444 data/dir_read_only
 
-echo '{"Valid" : ["Entry One", "Entry Two"]}' > data/mru_ok.json
+echo '{"Video" : ["Entry One", "Entry Two"]}' > data/mru_ok.json
 
-echo '{"Invalid" : [1, 3]}' > data/mru_invalid.json
+echo '{"Video" : [1, 3]}' > data/mru_invalid.json
 
 printf %s '1234567890' > data/ten_bytes
 touch -r $0 data/touch_mod_time

tests/tests/mru.cpp

@@ -12,99 +12,129 @@
 // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
-#include <libaegisub/fs.h>
 #include <libaegisub/mru.h>
 
+#include <libaegisub/fs.h>
+#include <libaegisub/option.h>
+#include <libaegisub/option_value.h>
+
 #include <main.h>
 
-static const char default_mru[] = "{\"Valid\" : []}";
+static const char default_mru[] = "{\"Video\" : []}";
+static const char conf_ok[] = "data/mru_ok.json";
 
-class lagi_mru : public libagi {
+TEST(lagi_mru, load_from_file) {
-protected:
-	std::string conf_ok;
-
-	void SetUp() override {
-		conf_ok = "./data/mru_ok.json";
-	}
-};
-
-
-TEST_F(lagi_mru, load_from_file) {
 	ASSERT_NO_THROW(agi::MRUManager mru(conf_ok, default_mru));
 	agi::MRUManager mru(conf_ok, default_mru);
-	ASSERT_NO_THROW(mru.Get("Valid"));
+	ASSERT_NO_THROW(mru.Get("Video"));
-	ASSERT_EQ(2u, mru.Get("Valid")->size());
+	ASSERT_EQ(2u, mru.Get("Video")->size());
-	auto entry = mru.Get("Valid")->begin();
+	auto entry = mru.Get("Video")->begin();
 	EXPECT_STREQ("Entry One", (*entry++).string().c_str());
 	EXPECT_STREQ("Entry Two", (*entry++).string().c_str());
-	EXPECT_TRUE(mru.Get("Valid")->end() == entry);
+	EXPECT_TRUE(mru.Get("Video")->end() == entry);
 }
 
-TEST_F(lagi_mru, load_from_default_string) {
+TEST(lagi_mru, load_from_default_string) {
 	agi::fs::Remove("data/mru_tmp");
 	agi::MRUManager mru("data/mru_tmp", default_mru);
 }
 
-TEST_F(lagi_mru, load_from_invalid_file) {
+TEST(lagi_mru, load_from_invalid_file) {
 	agi::fs::Copy("data/mru_invalid.json", "data/mru_tmp");
 	agi::MRUManager mru("data/mru_tmp", default_mru);
-	EXPECT_TRUE(mru.Get("Invalid")->empty());
+	EXPECT_TRUE(mru.Get("Video")->empty());
 }
 
-TEST_F(lagi_mru, add_entry) {
+TEST(lagi_mru, add_entry) {
 	agi::fs::Copy("data/mru_ok.json", "data/mru_tmp");
 	agi::MRUManager mru("data/mru_tmp", default_mru);
-	EXPECT_NO_THROW(mru.Add("Valid", "/path/to/file"));
+	EXPECT_NO_THROW(mru.Add("Video", "/path/to/file"));
-	EXPECT_STREQ("/path/to/file", mru.Get("Valid")->front().string().c_str());
+	EXPECT_STREQ("/path/to/file", mru.Get("Video")->front().string().c_str());
 }
 
-TEST_F(lagi_mru, remove_entry) {
+TEST(lagi_mru, remove_entry) {
 	agi::fs::Copy("data/mru_ok.json", "data/mru_tmp");
 	agi::MRUManager mru("data/mru_tmp", default_mru);
-	EXPECT_NO_THROW(mru.Add("Valid", "/path/to/file"));
+	EXPECT_NO_THROW(mru.Add("Video", "/path/to/file"));
-	EXPECT_NO_THROW(mru.Remove("Valid", "/path/to/file"));
+	EXPECT_NO_THROW(mru.Remove("Video", "/path/to/file"));
-	EXPECT_STRNE("/path/to/file", mru.Get("Valid")->front().string().c_str());
+	EXPECT_STRNE("/path/to/file", mru.Get("Video")->front().string().c_str());
 }
 
-TEST_F(lagi_mru, invalid_mru_key_throws) {
+TEST(lagi_mru, invalid_mru_key_throws) {
 	agi::fs::Copy("data/mru_ok.json", "data/mru_tmp");
 	agi::MRUManager mru("data/mru_tmp", default_mru);
-	EXPECT_THROW(mru.Add("Invalid", "/path/to/file"), agi::MRUErrorInvalidKey);
+	EXPECT_THROW(mru.Add("Invalid", "/path/to/file"), agi::MRUError);
-	EXPECT_THROW(mru.Get("Invalid"), agi::MRUErrorInvalidKey);
+	EXPECT_THROW(mru.Get("Invalid"), agi::MRUError);
 }
 
-TEST_F(lagi_mru, valid_mru_key_doesnt_throw) {
+TEST(lagi_mru, valid_mru_key_doesnt_throw) {
 	agi::fs::Copy("data/mru_ok.json", "data/mru_tmp");
 	agi::MRUManager mru("data/mru_tmp", default_mru);
-	EXPECT_NO_THROW(mru.Add("Valid", "/path/to/file"));
+	EXPECT_NO_THROW(mru.Add("Video", "/path/to/file"));
 }
 
-TEST_F(lagi_mru, adding_existing_moves_to_front) {
+TEST(lagi_mru, adding_existing_moves_to_front) {
 	agi::fs::Remove("data/mru_tmp");
 	agi::MRUManager mru("data/mru_tmp", default_mru);
 
-	EXPECT_NO_THROW(mru.Add("Valid", "/file/1"));
+	EXPECT_NO_THROW(mru.Add("Video", "/file/1"));
-	EXPECT_NO_THROW(mru.Add("Valid", "/file/2"));
+	EXPECT_NO_THROW(mru.Add("Video", "/file/2"));
-	EXPECT_NO_THROW(mru.Add("Valid", "/file/3"));
+	EXPECT_NO_THROW(mru.Add("Video", "/file/3"));
-	EXPECT_NO_THROW(mru.Add("Valid", "/file/1"));
+	EXPECT_NO_THROW(mru.Add("Video", "/file/1"));
-	EXPECT_NO_THROW(mru.Add("Valid", "/file/1"));
+	EXPECT_NO_THROW(mru.Add("Video", "/file/1"));
-	EXPECT_NO_THROW(mru.Add("Valid", "/file/3"));
+	EXPECT_NO_THROW(mru.Add("Video", "/file/3"));
 
-	EXPECT_STREQ("/file/3", mru.GetEntry("Valid", 0).string().c_str());
+	EXPECT_STREQ("/file/3", mru.GetEntry("Video", 0).string().c_str());
-	EXPECT_STREQ("/file/1", mru.GetEntry("Valid", 1).string().c_str());
+	EXPECT_STREQ("/file/1", mru.GetEntry("Video", 1).string().c_str());
-	EXPECT_STREQ("/file/2", mru.GetEntry("Valid", 2).string().c_str());
+	EXPECT_STREQ("/file/2", mru.GetEntry("Video", 2).string().c_str());
-	EXPECT_THROW(mru.GetEntry("Valid", 3), agi::MRUErrorIndexOutOfRange);
+	EXPECT_THROW(mru.GetEntry("Video", 3), agi::MRUError);
+}
+
+TEST(lagi_mru, all_valid_keys_work) {
+	agi::fs::Remove("data/mru_tmp");
+	agi::MRUManager mru("data/mru_tmp", default_mru);
+	EXPECT_NO_THROW(mru.Add("Audio", "/file"));
+	EXPECT_NO_THROW(mru.Add("Find", "/file"));
+	EXPECT_NO_THROW(mru.Add("Keyframes", "/file"));
+	EXPECT_NO_THROW(mru.Add("Replace", "/file"));
+	EXPECT_NO_THROW(mru.Add("Subtitle", "/file"));
+	EXPECT_NO_THROW(mru.Add("Timecodes", "/file"));
+	EXPECT_NO_THROW(mru.Add("Video", "/file"));
+}
+
+TEST(lagi_mru, prune_works) {
+	agi::fs::Remove("data/mru_tmp");
+	agi::MRUManager mru("data/mru_tmp", default_mru);
+
+	for (int i = 0; i < 20; ++i) {
+		ASSERT_NO_THROW(mru.Add("Audio", std::to_string(i)));
+	}
+
+	EXPECT_EQ(16, mru.Get("Audio")->size());
+	EXPECT_EQ("19", mru.Get("Audio")->front());
+}
+
+TEST(lagi_mru, prune_obeys_option) {
+	static const char opt[] = R"raw({"Limits": {"MRU": 1}})raw";
+	agi::Options options("", opt, agi::Options::FLUSH_SKIP);
+
+	agi::fs::Remove("data/mru_tmp");
+	agi::MRUManager mru("data/mru_tmp", {default_mru, sizeof(default_mru)}, &options);
+
+	ASSERT_NO_THROW(mru.Add("Audio", "1"));
+	ASSERT_NO_THROW(mru.Add("Audio", "2"));
+
+	EXPECT_EQ(1, mru.Get("Audio")->size());
 }
 
 // Check to make sure an entry is really removed.  This was fixed in
 // r4347, the entry was being removed from a copy of the map internally.
-TEST_F(lagi_mru, MRUEntryRemove_r4347) {
+TEST(lagi_mru, MRUEntryRemove_r4347) {
-
 	agi::MRUManager mru(conf_ok, default_mru);
-	EXPECT_NO_THROW(mru.Add("Valid", "/path/to/file"));
+	EXPECT_NO_THROW(mru.Add("Video", "/path/to/file"));
-	EXPECT_NO_THROW(mru.Remove("Valid", "/path/to/file"));
+	EXPECT_NO_THROW(mru.Remove("Video", "/path/to/file"));
 
-	const agi::MRUManager::MRUListMap *map_list = mru.Get("Valid");
+	const agi::MRUManager::MRUListMap *map_list = mru.Get("Video");
 	auto i_lst = map_list->begin();
 
 	if ((i_lst != map_list->end()) && (*i_lst == "/path/to/file"))