From bc410a99f69573d8757a7397ea7516db6d3e2f03 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Fri, 27 Jun 2014 18:22:58 -0700 Subject: [PATCH] Change the in-memory storage of options to a sorted vector --- build/libaegisub/libaegisub.vcxproj | 2 - build/libaegisub/libaegisub.vcxproj.filters | 6 - libaegisub/Makefile | 1 - libaegisub/common/option.cpp | 243 +++++++++++++++---- libaegisub/common/option_visit.cpp | 158 ------------ libaegisub/common/option_visit.h | 62 ----- libaegisub/include/libaegisub/option.h | 11 +- libaegisub/include/libaegisub/option_value.h | 2 +- tests/tests/option.cpp | 10 + 9 files changed, 213 insertions(+), 282 deletions(-) delete mode 100644 libaegisub/common/option_visit.cpp delete mode 100644 libaegisub/common/option_visit.h diff --git a/build/libaegisub/libaegisub.vcxproj b/build/libaegisub/libaegisub.vcxproj index 367d9bd0c..a5e51e28b 100644 --- a/build/libaegisub/libaegisub.vcxproj +++ b/build/libaegisub/libaegisub.vcxproj @@ -56,7 +56,6 @@ - @@ -141,7 +140,6 @@ - diff --git a/build/libaegisub/libaegisub.vcxproj.filters b/build/libaegisub/libaegisub.vcxproj.filters index 8244730c2..a9dfeba9c 100644 --- a/build/libaegisub/libaegisub.vcxproj.filters +++ b/build/libaegisub/libaegisub.vcxproj.filters @@ -32,9 +32,6 @@ Header Files - - Header Files - Header Files @@ -235,9 +232,6 @@ Source Files\Common - - Source Files\Common - Source Files\Common diff --git a/libaegisub/Makefile b/libaegisub/Makefile index 288d5988d..52c55290c 100644 --- a/libaegisub/Makefile +++ b/libaegisub/Makefile @@ -26,7 +26,6 @@ aegisub_OBJ := \ $(d)common/log.o \ $(d)common/mru.o \ $(d)common/option.o \ - $(d)common/option_visit.o \ $(d)common/path.o \ $(d)common/thesaurus.o \ $(d)common/util.o \ diff --git a/libaegisub/common/option.cpp b/libaegisub/common/option.cpp index 4c5a28169..71a496e1c 100644 --- a/libaegisub/common/option.cpp +++ b/libaegisub/common/option.cpp @@ -21,45 +21,165 @@ #include "libaegisub/cajun/reader.h" #include "libaegisub/cajun/writer.h" #include "libaegisub/cajun/elements.h" +#include "libaegisub/cajun/visitor.h" #include "libaegisub/exception.h" #include "libaegisub/fs.h" #include "libaegisub/io.h" #include "libaegisub/log.h" #include "libaegisub/option_value.h" +#include "libaegisub/make_unique.h" -#include "option_visit.h" - +#include #include #include #include namespace { - /// @brief Write an option to a json object - /// @param[out] obj Parent object - /// @param[in] path Path option should be stored in. - /// @param[in] value Value to write. - void put_option(json::Object &obj, const std::string &path, const json::UnknownElement &value) { - std::string::size_type pos = path.find('/'); - // Not having a '/' denotes it is a leaf. - if (pos == std::string::npos) { - assert(obj.find(path) == obj.end()); - obj[path] = value; - } +using namespace agi; + +DEFINE_EXCEPTION(OptionJsonValueError, Exception); + +class ConfigVisitor final : public json::ConstVisitor { + std::vector> values; + + /// Option name prefix to add to read names + std::string name; + + /// Log errors rather than throwing them, for when loading user config files + /// (as a bad user config file shouldn't make the program fail to start) + bool ignore_errors; + + void Error(const char *message) { + if (ignore_errors) + LOG_E("option/load/config_visitor") << "Error loading option from user configuration: " << message; else - put_option(obj[path.substr(0, pos)], path.substr(pos + 1), value); + throw OptionJsonValueError(message); } - template - void put_array(json::Object &obj, const std::string &path, const char *element_key, std::vector const& value) { - json::Array array; - for (T const& elem : value) { - array.push_back(json::Object()); - static_cast(array.back())[element_key] = (json::UnknownElement)elem; + template + void ReadArray(json::Array const& src, std::string const& array_type) { + typename OptionValueType::value_type arr; + arr.reserve(src.size()); + + for (json::Object const& obj : src) { + if (obj.size() != 1) + return Error("Invalid array member"); + if (obj.begin()->first != array_type) + return Error("Attempt to insert value into array of wrong type"); + + arr.push_back((typename OptionValueType::value_type::value_type)(obj.begin()->second)); } - put_option(obj, path, array); + values.push_back(agi::make_unique(name, std::move(arr))); } + + void Visit(const json::Object& object) { + auto old_name = name; + for (auto const& obj : object) { + name = old_name + (name.empty() ? "" : "/") + obj.first; + obj.second.Accept(*this); + } + name = old_name; + } + + void Visit(const json::Array& array) { + if (array.empty()) + return Error("Cannot infer the type of an empty array"); + + json::Object const& front = array.front(); + if (front.size() != 1) + return Error("Invalid array member"); + + auto const& array_type = front.begin()->first; + if (array_type == "string") + ReadArray(array, array_type); + else if (array_type == "int") + ReadArray(array, array_type); + else if (array_type == "double") + ReadArray(array, array_type); + else if (array_type == "bool") + ReadArray(array, array_type); + else if (array_type == "color") + ReadArray(array, array_type); + else + Error("Array type not handled"); + } + + void Visit(const json::Integer& number) { + values.push_back(agi::make_unique(name, number)); + } + + void Visit(const json::Double& number) { + values.push_back(agi::make_unique(name, number)); + } + + void Visit(const json::String& string) { + size_t size = string.size(); + if ((size == 4 && string[0] == '#') || + (size == 7 && string[0] == '#') || + (size >= 10 && boost::starts_with(string, "rgb(")) || + ((size == 9 || size == 10) && boost::starts_with(string, "&H"))) + { + values.push_back(agi::make_unique(name, string)); + } else { + values.push_back(agi::make_unique(name, string)); + } + } + + void Visit(const json::Boolean& boolean) { + values.push_back(agi::make_unique(name, boolean)); + } + + void Visit(const json::Null& null) { + Error("Attempt to read null value"); + } + +public: + ConfigVisitor(bool ignore_errors) : ignore_errors(ignore_errors) { } + std::vector> Values() { return std::move(values); } +}; + +/// @brief Write an option to a json object +/// @param[out] obj Parent object +/// @param[in] path Path option should be stored in. +/// @param[in] value Value to write. +void put_option(json::Object &obj, const std::string &path, const json::UnknownElement &value) { + std::string::size_type pos = path.find('/'); + // Not having a '/' denotes it is a leaf. + if (pos == std::string::npos) { + assert(obj.find(path) == obj.end()); + obj[path] = value; + } + else + put_option(obj[path.substr(0, pos)], path.substr(pos + 1), value); +} + +template +void put_array(json::Object &obj, const std::string &path, const char *element_key, std::vector const& value) { + json::Array array; + for (T const& elem : value) { + array.push_back(json::Object()); + static_cast(array.back())[element_key] = (json::UnknownElement)elem; + } + + put_option(obj, path, array); +} + +struct option_name_cmp { + bool operator()(std::unique_ptr const& a, std::unique_ptr const& b) const { + return a->GetName() < b->GetName(); + } + + bool operator()(std::unique_ptr const& a, std::string const& b) const { + return a->GetName() < b; + } + + bool operator()(std::unique_ptr const& a, const char *b) const { + return a->GetName() < b; + } +}; + } namespace agi { @@ -78,19 +198,13 @@ Options::~Options() { Flush(); } -void Options::ConfigNext(std::istream& stream) { - LoadConfig(stream); -} - void Options::ConfigUser() { try { - std::unique_ptr stream(io::Open(config_file)); - LoadConfig(*stream, true); + LoadConfig(*io::Open(config_file), true); } catch (fs::FileNotFound const&) { return; } - /// @todo Handle other errors such as parsing and notifying the user. } void Options::LoadConfig(std::istream& stream, bool ignore_errors) { @@ -100,67 +214,104 @@ void Options::LoadConfig(std::istream& stream, bool ignore_errors) { json::Reader::Read(config_root, stream); } catch (json::Reader::ParseException& e) { LOG_E("option/load") << "json::ParseException: " << e.what() << ", Line/offset: " << e.m_locTokenBegin.m_nLine + 1 << '/' << e.m_locTokenBegin.m_nLineOffset + 1; + return; } catch (json::Exception& e) { - /// @todo Do something better here, maybe print the exact error LOG_E("option/load") << "json::Exception: " << e.what(); + return; } - ConfigVisitor config_visitor(values, "", ignore_errors, !ignore_errors); + ConfigVisitor config_visitor(ignore_errors); config_root.Accept(config_visitor); + + auto new_values = config_visitor.Values(); + if (new_values.empty()) return; + + sort(begin(new_values), end(new_values), option_name_cmp()); + + if (values.empty()) { + values = std::move(new_values); + return; + } + + auto src_it = begin(new_values), src_end = end(new_values); + auto dst_it = begin(values), dst_end = end(values); + + while (src_it != src_end && dst_it != dst_end) { + int cmp = (*src_it)->GetName().compare((*dst_it)->GetName()); + if (cmp < 0) // Option doesn't exist in defaults so skip + ++src_it; + else if (cmp > 0) + ++dst_it; + else { + if (ignore_errors) { + try { + (*dst_it)->Set((*src_it).get()); + } + catch (agi::InternalError const& e) { + LOG_E("option/load/config_visitor") << "Error loading option from user configuration: " << e.GetMessage(); + } + } + else { + *dst_it = std::move(*src_it); + } + ++src_it; + ++dst_it; + } + } } -OptionValue* Options::Get(const std::string &name) { - auto index = values.find(name); - if (index != values.end()) - return index->second.get(); +OptionValue *Options::Get(const char *name) { + auto index = lower_bound(begin(values), end(values), name, option_name_cmp()); + if (index != end(values) && (*index)->GetName() == name) + return index->get(); LOG_E("option/get") << "agi::Options::Get Option not found: (" << name << ")"; - throw agi::InternalError("Option value not found: " + name); + throw agi::InternalError("Option value not found: " + std::string(name)); } void Options::Flush() const { json::Object obj_out; for (auto const& ov : values) { - switch (ov.second->GetType()) { + switch (ov->GetType()) { case OptionType::String: - put_option(obj_out, ov.first, ov.second->GetString()); + put_option(obj_out, ov->GetName(), ov->GetString()); break; case OptionType::Int: - put_option(obj_out, ov.first, ov.second->GetInt()); + put_option(obj_out, ov->GetName(), ov->GetInt()); break; case OptionType::Double: - put_option(obj_out, ov.first, ov.second->GetDouble()); + put_option(obj_out, ov->GetName(), ov->GetDouble()); break; case OptionType::Color: - put_option(obj_out, ov.first, ov.second->GetColor().GetRgbFormatted()); + put_option(obj_out, ov->GetName(), ov->GetColor().GetRgbFormatted()); break; case OptionType::Bool: - put_option(obj_out, ov.first, ov.second->GetBool()); + put_option(obj_out, ov->GetName(), ov->GetBool()); break; case OptionType::ListString: - put_array(obj_out, ov.first, "string", ov.second->GetListString()); + put_array(obj_out, ov->GetName(), "string", ov->GetListString()); break; case OptionType::ListInt: - put_array(obj_out, ov.first, "int", ov.second->GetListInt()); + put_array(obj_out, ov->GetName(), "int", ov->GetListInt()); break; case OptionType::ListDouble: - put_array(obj_out, ov.first, "double", ov.second->GetListDouble()); + put_array(obj_out, ov->GetName(), "double", ov->GetListDouble()); break; case OptionType::ListColor: - put_array(obj_out, ov.first, "color", ov.second->GetListColor()); + put_array(obj_out, ov->GetName(), "color", ov->GetListColor()); break; case OptionType::ListBool: - put_array(obj_out, ov.first, "bool", ov.second->GetListBool()); + put_array(obj_out, ov->GetName(), "bool", ov->GetListBool()); break; } } diff --git a/libaegisub/common/option_visit.cpp b/libaegisub/common/option_visit.cpp deleted file mode 100644 index 7cb9f25fd..000000000 --- a/libaegisub/common/option_visit.cpp +++ /dev/null @@ -1,158 +0,0 @@ -// Copyright (c) 2010, Amar Takhar -// -// Permission to use, copy, modify, and distribute this software for any -// purpose with or without fee is hereby granted, provided that the above -// copyright notice and this permission notice appear in all copies. -// -// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES -// WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF -// MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR -// ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES -// WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN -// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF -// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. - -/// @file option_visit.cpp -/// @brief Cajun JSON visitor to load config values. -/// @see option_visit.h -/// @ingroup libaegisub - -#include "option_visit.h" - -#include - -#include -#include -#include - -#include - -namespace agi { - -ConfigVisitor::ConfigVisitor(OptionValueMap &val, const std::string &member_name, bool ignore_errors, bool replace) -: values(val) -, name(member_name) -, ignore_errors(ignore_errors) -, replace(replace) -{ -} - -void ConfigVisitor::Error(const char *message) { - if (ignore_errors) - LOG_E("option/load/config_visitor") << "Error loading option from user configuration: " << message; - else - throw OptionJsonValueError(message); -} - -void ConfigVisitor::Visit(const json::Object& object) { - if (!name.empty()) - name += "/"; - - for (auto const& obj : object) { - ConfigVisitor config_visitor(values, name + obj.first, ignore_errors, replace); - obj.second.Accept(config_visitor); - } -} - -template -std::unique_ptr ConfigVisitor::ReadArray(json::Array const& src, std::string const& array_type) { - typename OptionValueType::value_type arr; - arr.reserve(src.size()); - - for (json::Object const& obj : src) { - if (obj.size() != 1) { - Error("Invalid array member"); - return nullptr; - } - if (obj.begin()->first != array_type) { - Error("Attempt to insert value into array of wrong type"); - return nullptr; - } - - arr.push_back((typename OptionValueType::value_type::value_type)(obj.begin()->second)); - } - - return agi::make_unique(name, std::move(arr)); -} - -void ConfigVisitor::Visit(const json::Array& array) { - if (array.empty()) { - Error("Cannot infer the type of an empty array"); - return; - } - - json::Object const& front = array.front(); - if (front.size() != 1) { - Error("Invalid array member"); - return; - } - - const std::string& array_type = front.begin()->first; - - if (array_type == "string") - AddOptionValue(ReadArray(array, array_type)); - else if (array_type == "int") - AddOptionValue(ReadArray(array, array_type)); - else if (array_type == "double") - AddOptionValue(ReadArray(array, array_type)); - else if (array_type == "bool") - AddOptionValue(ReadArray(array, array_type)); - else if (array_type == "color") - AddOptionValue(ReadArray(array, array_type)); - else - Error("Array type not handled"); -} - -void ConfigVisitor::Visit(const json::Integer& number) { - AddOptionValue(agi::make_unique(name, number)); -} - -void ConfigVisitor::Visit(const json::Double& number) { - AddOptionValue(agi::make_unique(name, number)); -} - -void ConfigVisitor::Visit(const json::String& string) { - size_t size = string.size(); - if ((size == 4 && string[0] == '#') || - (size == 7 && string[0] == '#') || - (size >= 10 && boost::starts_with(string, "rgb(")) || - ((size == 9 || size == 10) && boost::starts_with(string, "&H"))) - { - AddOptionValue(agi::make_unique(name, string)); - } else { - AddOptionValue(agi::make_unique(name, string)); - } -} - -void ConfigVisitor::Visit(const json::Boolean& boolean) { - AddOptionValue(agi::make_unique(name, boolean)); -} - -void ConfigVisitor::Visit(const json::Null& null) { - Error("Attempt to read null value"); -} - -void ConfigVisitor::AddOptionValue(std::unique_ptr&& opt) { - if (!opt) { - assert(ignore_errors); - return; - } - - auto it = values.find(name); - if (it == values.end()) - values[name] = std::move(opt); - else if (replace) - it->second = std::move(opt); - else { - try { - values[name]->Set(opt.get()); - } - catch (agi::InternalError const& e) { - if (ignore_errors) - LOG_E("option/load/config_visitor") << "Error loading option from user configuration: " << e.GetMessage(); - else - throw; - } - } -} -} // namespace agi diff --git a/libaegisub/common/option_visit.h b/libaegisub/common/option_visit.h deleted file mode 100644 index 4362f7130..000000000 --- a/libaegisub/common/option_visit.h +++ /dev/null @@ -1,62 +0,0 @@ -// Copyright (c) 2010, Amar Takhar -// -// Permission to use, copy, modify, and distribute this software for any -// purpose with or without fee is hereby granted, provided that the above -// copyright notice and this permission notice appear in all copies. -// -// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES -// WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF -// MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR -// ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES -// WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN -// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF -// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. - -/// @file option_visit.h -/// @brief Cajun JSON visitor to load config values. -/// @see option_visit.cpp -/// @ingroup libaegisub - -#include "libaegisub/option.h" -#include "libaegisub/cajun/elements.h" -#include "libaegisub/cajun/visitor.h" - -#include "libaegisub/exception.h" - -#include - -namespace agi { - -DEFINE_EXCEPTION(OptionJsonValueError, Exception); - -class ConfigVisitor final : public json::ConstVisitor { - /// Option map being populated - OptionValueMap &values; - /// Option name prefix to add to read names - std::string name; - /// Log errors rather than throwing them, for when loading user config files - /// (as a bad user config file shouldn't make the program fail to start) - bool ignore_errors; - /// Replace existing options rather than changing their value, so that the - /// default value is changed to the new one - bool replace; - - void Error(const char *message); - - template - std::unique_ptr ReadArray(json::Array const& src, std::string const& array_type); - - void AddOptionValue(std::unique_ptr&& opt); -public: - ConfigVisitor(OptionValueMap &val, const std::string &member_name, bool ignore_errors = false, bool replace = false); - - void Visit(const json::Array& array); - void Visit(const json::Object& object); - void Visit(const json::Integer& number); - void Visit(const json::Double& number); - void Visit(const json::String& string); - void Visit(const json::Boolean& boolean); - void Visit(const json::Null& null); -}; - -} // namespace agi diff --git a/libaegisub/include/libaegisub/option.h b/libaegisub/include/libaegisub/option.h index 1406b7cc7..eb94ef670 100644 --- a/libaegisub/include/libaegisub/option.h +++ b/libaegisub/include/libaegisub/option.h @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -33,8 +34,6 @@ namespace json { namespace agi { class OptionValue; -using OptionValueMap = std::map>; - class Options { public: /// Options class settings. @@ -44,8 +43,7 @@ public: }; private: - /// Internal OptionValueMap - OptionValueMap values; + std::vector> values; /// User config (file that will be written to disk) const agi::fs::path config_file; @@ -74,14 +72,15 @@ public: /// @brief Get an option by name. /// @param name Option to get. /// Get an option value object by name throw an internal exception if the option is not found. - OptionValue* Get(const std::string &name); + OptionValue *Get(const char *name); + OptionValue *Get(std::string const& name) { return Get(name.c_str()); } /// @brief Next configuration file to load. /// @param[in] src Stream to load from. /// Load next config which will supersede any values from previous configs /// can be called as many times as required, but only after ConfigDefault() and /// before ConfigUser() - void ConfigNext(std::istream &stream); + void ConfigNext(std::istream &stream) { LoadConfig(stream); } /// @brief Set user config file. /// Set the user configuration file and read options from it, closes all diff --git a/libaegisub/include/libaegisub/option_value.h b/libaegisub/include/libaegisub/option_value.h index 0c088a6be..b70a0d11e 100644 --- a/libaegisub/include/libaegisub/option_value.h +++ b/libaegisub/include/libaegisub/option_value.h @@ -90,7 +90,7 @@ protected: public: virtual ~OptionValue() = default; - std::string GetName() const { return name; } + std::string const& GetName() const { return name; } virtual OptionType GetType() const = 0; virtual bool IsDefault() const = 0; virtual void Reset() = 0; diff --git a/tests/tests/option.cpp b/tests/tests/option.cpp index 1bdd39a11..efa8d45bf 100644 --- a/tests/tests/option.cpp +++ b/tests/tests/option.cpp @@ -120,6 +120,16 @@ TEST_F(lagi_option, heterogeneous_arrays_rejected) { EXPECT_THROW(agi::Options("", "{ \"key\" : [ { \"bool\" : true }, { \"double\" : 1.0 } ] }", agi::Options::FLUSH_SKIP), agi::Exception); } +TEST_F(lagi_option, set_works) { + agi::Options opt("", all_types, agi::Options::FLUSH_SKIP); + + ASSERT_NO_THROW(opt.Get("Integer")->SetInt(1000)); + EXPECT_EQ(1000, opt.Get("Integer")->GetInt()); + + ASSERT_NO_THROW(opt.Get("String")->SetString("Hello")); + EXPECT_EQ("Hello", opt.Get("String")->GetString()); +} + TEST_F(lagi_option, flush_roundtrip) { agi::fs::Remove("data/options/tmp");