Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(config_compiler): "/" mistaken as path separator in merged map key #192

Merged
merged 2 commits into from
Mar 7, 2018
Merged

fix(config_compiler): "/" mistaken as path separator in merged map key #192

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f35cae
fix(config_compiler): "/" mistaken as path separator in merged map key
kionz Mar 7, 2018
3c3f905
refactor(config_compiler): no more need the lambda
kionz Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/rime/config/config_compiler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,17 +155,22 @@ inline static string StripOperator(const string& key, bool adding) {
}

// defined in config_data.cc
bool TraverseCopyOnWrite(an<ConfigItemRef> root, const string& path,
function<bool (an<ConfigItemRef> target)> writer);
an<ConfigItemRef> TypeCheckedCopyOnWrite(an<ConfigItemRef> parent,
const string& key);
an<ConfigItemRef> TraverseCopyOnWrite(an<ConfigItemRef> root,
const string& path);

static bool EditNode(an<ConfigItemRef> target,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name target is used for another (argument) variable in lambda, which can refer to a different config node. Better differentiate the name.

const string& key,
const an<ConfigItem>& 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<ConfigItemRef> target) {
if (!target) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you null-check the result of cow_node(target, path), this would be unnecessary.

return false;
}
if ((appending || merging) && **target) {
DLOG(INFO) << "writer: editing node";
return !value ||
Expand All @@ -181,7 +186,8 @@ static bool EditNode(an<ConfigItemRef> 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name sounds like a "node" but it's not. Name it using a verb phase, for example "find_target_node"?

return writer(cow_node(target, path));
}

bool PatchLiteral::Resolve(ConfigCompiler* compiler) {
Expand Down
42 changes: 29 additions & 13 deletions src/rime/config/config_data.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,37 +159,53 @@ class ConfigDataRootRef : public ConfigItemRef {
ConfigData* data_;
};

bool TraverseCopyOnWrite(an<ConfigItemRef> root, const string& path,
function<bool (an<ConfigItemRef> target)> writer) {
an<ConfigItemRef> TypeCheckedCopyOnWrite(an<ConfigItemRef> 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<ConfigItem> 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<ConfigItemRef> TraverseCopyOnWrite(an<ConfigItemRef> root,
const string& path) {
DLOG(INFO) << "TraverseCopyOnWrite(" << path << ")";
if (path.empty() || path == "/") {
return writer(root);
return root;
}
an<ConfigItemRef> head = root;
vector<string> 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<ConfigItem> 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<ConfigItem> item) {
LOG(INFO) << "write: " << path;
auto root = New<ConfigDataRootRef>(this);
return TraverseCopyOnWrite(root, path, [=](an<ConfigItemRef> target) {
if (auto target = TraverseCopyOnWrite(root, path)) {
*target = item;
set_modified();
return true;
});
} else {
return false;
}
}

vector<string> ConfigData::SplitPath(const string& path) {
Expand Down