From 0f35caebb72a9f4aa6556079addeaa6cc4a5006d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B1=85=E6=88=8E=E6=B0=8F?= Date: Wed, 7 Mar 2018 22:25:10 +0800 Subject: [PATCH 1/2] fix(config_compiler): "/" mistaken as path separator in merged map key --- src/rime/config/config_compiler.cc | 14 +++++++--- src/rime/config/config_data.cc | 42 +++++++++++++++++++++--------- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc index 8af3de032..206e71fc1 100644 --- a/src/rime/config/config_compiler.cc +++ b/src/rime/config/config_compiler.cc @@ -155,17 +155,22 @@ inline static string StripOperator(const string& key, bool adding) { } // defined in config_data.cc -bool TraverseCopyOnWrite(an root, const string& path, - function target)> writer); +an TypeCheckedCopyOnWrite(an parent, + const string& key); +an TraverseCopyOnWrite(an root, + const string& path); static bool EditNode(an target, const string& key, const an& value, bool merge_tree) { - DLOG(INFO) << "EditNode(" << key << "," << merge_tree << ")"; + DLOG(INFO) << "edit node: " << key << ", merge_tree: " << merge_tree; bool appending = IsAppending(key); bool merging = IsMerging(key, value, merge_tree); auto writer = [=](an target) { + if (!target) { + return false; + } if ((appending || merging) && **target) { DLOG(INFO) << "writer: editing node"; return !value || @@ -181,7 +186,8 @@ static bool EditNode(an target, string path = StripOperator(key, appending || merging); DLOG(INFO) << "appending: " << appending << ", merging: " << merging << ", path: " << path; - return TraverseCopyOnWrite(target, path, writer); + auto cow_node = merge_tree ? &TypeCheckedCopyOnWrite : &TraverseCopyOnWrite; + return writer(cow_node(target, path)); } bool PatchLiteral::Resolve(ConfigCompiler* compiler) { diff --git a/src/rime/config/config_data.cc b/src/rime/config/config_data.cc index 980c1042d..ce2a5f2b9 100644 --- a/src/rime/config/config_data.cc +++ b/src/rime/config/config_data.cc @@ -159,37 +159,53 @@ class ConfigDataRootRef : public ConfigItemRef { ConfigData* data_; }; -bool TraverseCopyOnWrite(an root, const string& path, - function target)> writer) { +an TypeCheckedCopyOnWrite(an parent, + const string& key) { + // special case to allow editing current node by __append: __merge: /+: /=: + if (key.empty()) { + return parent; + } + bool is_list = ConfigData::IsListItemReference(key); + auto expected_node_type = is_list ? ConfigItem::kList : ConfigItem::kMap; + an existing_node = *parent; + if (existing_node && existing_node->type() != expected_node_type) { + LOG(ERROR) << "copy on write failed; incompatible node type: " << key; + return nullptr; + } + return Cow(parent, key); +} + +an TraverseCopyOnWrite(an root, + const string& path) { DLOG(INFO) << "TraverseCopyOnWrite(" << path << ")"; if (path.empty() || path == "/") { - return writer(root); + return root; } an head = root; vector keys = ConfigData::SplitPath(path); size_t n = keys.size(); for (size_t i = 0; i < n; ++i) { const auto& key = keys[i]; - bool is_list = ConfigData::IsListItemReference(key); - auto expected_node_type = is_list ? ConfigItem::kList : ConfigItem::kMap; - an existing_node = *head; - if (existing_node && existing_node->type() != expected_node_type) { - LOG(ERROR) << "copy on write failed; incompatible node type: " << key; - return false; + if (auto child = TypeCheckedCopyOnWrite(head, key)) { + head = child; + } else { + LOG(ERROR) << "while writing to " << path; + return nullptr; } - head = Cow(head, key); } - return writer(head); + return head; } bool ConfigData::TraverseWrite(const string& path, an item) { LOG(INFO) << "write: " << path; auto root = New(this); - return TraverseCopyOnWrite(root, path, [=](an target) { + if (auto target = TraverseCopyOnWrite(root, path)) { *target = item; set_modified(); return true; - }); + } else { + return false; + } } vector ConfigData::SplitPath(const string& path) { From 3c3f9056b0814fe04b9bccb4b1c448d4e9048f02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B1=85=E6=88=8E=E6=B0=8F?= Date: Wed, 7 Mar 2018 23:43:44 +0800 Subject: [PATCH 2/2] refactor(config_compiler): no more need the lambda --- src/rime/config/config_compiler.cc | 40 +++++++++++++++--------------- src/rime/config/config_data.cc | 5 ++-- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc index 206e71fc1..30557b077 100644 --- a/src/rime/config/config_compiler.cc +++ b/src/rime/config/config_compiler.cc @@ -157,37 +157,37 @@ inline static string StripOperator(const string& key, bool adding) { // defined in config_data.cc an TypeCheckedCopyOnWrite(an parent, const string& key); -an TraverseCopyOnWrite(an root, +an TraverseCopyOnWrite(an head, const string& path); -static bool EditNode(an target, +static bool EditNode(an head, const string& key, const an& value, bool merge_tree) { DLOG(INFO) << "edit node: " << key << ", merge_tree: " << merge_tree; bool appending = IsAppending(key); bool merging = IsMerging(key, value, merge_tree); - auto writer = [=](an target) { - if (!target) { - return false; - } - if ((appending || merging) && **target) { - DLOG(INFO) << "writer: editing node"; - return !value || - (appending && (AppendToString(target, As(value)) || - AppendToList(target, As(value)))) || - (merging && MergeTree(target, As(value))); - } else { - DLOG(INFO) << "writer: overwriting node"; - *target = value; - return true; - } - }; string path = StripOperator(key, appending || merging); DLOG(INFO) << "appending: " << appending << ", merging: " << merging << ", path: " << path; - auto cow_node = merge_tree ? &TypeCheckedCopyOnWrite : &TraverseCopyOnWrite; - return writer(cow_node(target, path)); + auto find_target_node = + merge_tree ? &TypeCheckedCopyOnWrite : &TraverseCopyOnWrite; + auto target = find_target_node(head, path); + if (!target) { + // error finding target node; cannot write + return false; + } + if ((appending || merging) && **target) { + DLOG(INFO) << "writer: editing node"; + return !value || // no-op + (appending && (AppendToString(target, As(value)) || + AppendToList(target, As(value)))) || + (merging && MergeTree(target, As(value))); + } else { + DLOG(INFO) << "writer: overwriting node"; + *target = value; + return true; + } } bool PatchLiteral::Resolve(ConfigCompiler* compiler) { diff --git a/src/rime/config/config_data.cc b/src/rime/config/config_data.cc index ce2a5f2b9..43783f147 100644 --- a/src/rime/config/config_data.cc +++ b/src/rime/config/config_data.cc @@ -175,13 +175,12 @@ an TypeCheckedCopyOnWrite(an parent, return Cow(parent, key); } -an TraverseCopyOnWrite(an root, +an TraverseCopyOnWrite(an head, const string& path) { DLOG(INFO) << "TraverseCopyOnWrite(" << path << ")"; if (path.empty() || path == "/") { - return root; + return head; } - an head = root; vector keys = ConfigData::SplitPath(path); size_t n = keys.size(); for (size_t i = 0; i < n; ++i) {