diff --git a/data/test/config_compiler_test.yaml b/data/test/config_compiler_test.yaml index 9a29bab78..408f67224 100644 --- a/data/test/config_compiler_test.yaml +++ b/data/test/config_compiler_test.yaml @@ -42,6 +42,16 @@ dependency_chaining: __include: /dependency_chaining/beta epsilon: success +dependency_priorities: + terrans: + __include: /starcraft/terrans + __patch: + player: nada + protoss: + __patch: + player: bisu + __include: /starcraft/protoss + local: patch: battlefields/@next: match point diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc index 8f98c1cd2..00851c335 100644 --- a/src/rime/config/config_compiler.cc +++ b/src/rime/config/config_compiler.cc @@ -7,17 +7,26 @@ namespace rime { +enum DependencyPriority { + kPendingChild = 0, + kInclude = 1, + kPatch = 2, +}; + struct Dependency { an target; - virtual bool blocking() const = 0; + virtual DependencyPriority priority() const = 0; + bool blocking() const { + return priority() > kPendingChild; + } virtual string repr() const = 0; virtual bool Resolve(ConfigCompiler* compiler) = 0; }; template StreamT& operator<< (StreamT& stream, const Dependency& dep) { - return stream << dep.repr() << (dep.blocking() ? "(blocking)" : ""); + return stream << dep.repr(); } struct PendingChild : Dependency { @@ -27,8 +36,8 @@ struct PendingChild : Dependency { PendingChild(const string& path, const an& ref) : child_path(path), child_ref(ref) { } - bool blocking() const override { - return false; + DependencyPriority priority() const override { + return kPendingChild; } string repr() const override { return "PendingChild(" + child_path + ")"; @@ -48,8 +57,8 @@ StreamT& operator<< (StreamT& stream, const Reference& reference) { struct IncludeReference : Dependency { IncludeReference(const Reference& r) : reference(r) { } - bool blocking() const override { - return true; + DependencyPriority priority() const override { + return kInclude; } string repr() const override { return "Include(" + reference.repr() + ")"; @@ -62,8 +71,8 @@ struct IncludeReference : Dependency { struct PatchReference : Dependency { PatchReference(const Reference& r) : reference(r) { } - bool blocking() const override { - return true; + DependencyPriority priority() const override { + return kPatch; } string repr() const override { return "Patch(" + reference.repr() + ")"; @@ -78,8 +87,8 @@ struct PatchLiteral : Dependency { PatchLiteral(an map) : patch(map) { } - bool blocking() const override { - return true; + DependencyPriority priority() const override { + return kPatch; } string repr() const override { return "Patch()"; @@ -91,7 +100,7 @@ struct ConfigDependencyGraph { map> resources; vector> node_stack; vector key_stack; - map>> deps; + map>> deps; void Add(an dependency); @@ -162,21 +171,34 @@ bool PatchLiteral::Resolve(ConfigCompiler* compiler) { return success; } +static void InsertByPriority(vector>& list, + const an& value) { + auto upper = std::upper_bound( + list.begin(), list.end(), value, + [](const an& lhs, const an& rhs) { + return lhs->priority() < rhs->priority(); + }); + list.insert(upper, value); +} + void ConfigDependencyGraph::Add(an dependency) { - LOG(INFO) << "ConfigDependencyGraph::Add(), node_stack.size() = " << node_stack.size(); + LOG(INFO) << "ConfigDependencyGraph::Add(), node_stack.size() = " + << node_stack.size(); if (node_stack.empty()) return; const auto& target = node_stack.back(); dependency->target = target; auto target_path = ConfigData::JoinPath(key_stack); auto& target_deps = deps[target_path]; bool target_was_pending = !target_deps.empty(); - target_deps.push_back(dependency); - LOG(INFO) << "target_path = " << target_path << ", #deps = " << target_deps.size(); + InsertByPriority(target_deps, dependency); + LOG(INFO) << "target_path = " << target_path + << ", #deps = " << target_deps.size(); if (target_was_pending || // so was all ancestors key_stack.size() == 1) { // this is the progenitor return; } - // The current pending node becomes a prioritized dependency of parent node + // The current pending node becomes a prioritized non-blocking dependency of + // its parent node; spread the pending state to its non-pending ancestors auto keys = key_stack; for (auto child = node_stack.rbegin(); child != node_stack.rend(); ++child) { auto last_key = keys.back(); @@ -185,8 +207,10 @@ void ConfigDependencyGraph::Add(an dependency) { auto& parent_deps = deps[parent_path]; bool parent_was_pending = !parent_deps.empty(); // Pending children should be resolved before applying __include or __patch - parent_deps.push_front(New(parent_path + "/" + last_key, *child)); - LOG(INFO) << "parent_path = " << parent_path << ", #deps = " << parent_deps.size(); + InsertByPriority(parent_deps, + New(parent_path + "/" + last_key, *child)); + LOG(INFO) << "parent_path = " << parent_path + << ", #deps = " << parent_deps.size(); if (parent_was_pending || // so was all ancestors keys.size() == 1) { // this parent is the progenitor return; @@ -418,10 +442,10 @@ bool ConfigCompiler::ResolveDependencies(const string& path) { auto& deps = graph_->deps[path]; for (auto iter = deps.begin(); iter != deps.end(); ) { if (!(*iter)->Resolve(this)) { - LOG(ERROR) << "unesolved dependency:" << **iter; + LOG(ERROR) << "unresolved dependency: " << **iter; return false; } - LOG(INFO) << "resolved " << **iter; + LOG(INFO) << "resolved: " << **iter; iter = deps.erase(iter); } LOG(INFO) << "all dependencies resolved."; diff --git a/test/config_compiler_test.cc b/test/config_compiler_test.cc index f7043d005..69990ddbd 100644 --- a/test/config_compiler_test.cc +++ b/test/config_compiler_test.cc @@ -115,4 +115,18 @@ TEST_F(RimeConfigCompilerTest, DependencyChaining) { EXPECT_EQ("success", value); } +// Unit test for https://github.com/rime/librime/issues/141 +TEST_F(RimeConfigCompilerTest, DependencyPriorities) { + const string& prefix = "dependency_priorities/"; + EXPECT_TRUE(config_->IsNull(prefix + "terrans/__include")); + EXPECT_TRUE(config_->IsNull(prefix + "terrans/__patch")); + EXPECT_TRUE(config_->IsNull(prefix + "protoss/__include")); + EXPECT_TRUE(config_->IsNull(prefix + "protoss/__patch")); + string player; + EXPECT_TRUE(config_->GetString(prefix + "terrans/player", &player)); + EXPECT_EQ("nada", player); + EXPECT_TRUE(config_->GetString(prefix + "protoss/player", &player)); + EXPECT_EQ("bisu", player); +} + // TODO: test failure cases