Simplify code produced by gn edit. When you remove entries from a list, this cleans up the list afterwards. Change-Id: I719ba89eba3cf3a25087bd48164a1cea6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26100 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/src/gn/build_file_editor.cc b/src/gn/build_file_editor.cc index 7800024..c3cecc4 100644 --- a/src/gn/build_file_editor.cc +++ b/src/gn/build_file_editor.cc
@@ -216,6 +216,26 @@ return results.front(); } +bool IsEmptyList(const ParseNode* node) { + auto* list = node->AsList(); + return list && list->contents().empty(); +} + +std::unique_ptr<ParseNode> SimplifyExpression(std::unique_ptr<ParseNode> expr) { + // We could recurse into minuses, but `gn edit` never modifies anything in a + // minus, so no need to simplify. + if (auto op = expr->AsBinaryOpMut(); op && op->op().type() == Token::PLUS) { + op->set_left(SimplifyExpression(op->take_left())); + op->set_right(SimplifyExpression(op->take_right())); + if (IsEmptyList(op->left())) { + return op->take_right(); + } else if (IsEmptyList(op->right())) { + return op->take_left(); + } + } + return expr; +} + TreeNode TreeNode::Descend(ParseNode* child) const { std::vector<ParseNode*> s = stack_; s.push_back(child);
diff --git a/src/gn/build_file_editor.h b/src/gn/build_file_editor.h index 90c5839..dd0ee0b 100644 --- a/src/gn/build_file_editor.h +++ b/src/gn/build_file_editor.h
@@ -149,6 +149,13 @@ // Finds the first list node within an assignment expression. std::optional<ListNode*> FindListInAssignment(const TreeNode& assignment); +// Returns whether the node represents an empty list. +bool IsEmptyList(const ParseNode* node); + +// Simplifies expressions by removing redundant elements. +// eg. [] + ["//foo"] => ["//foo"] +std::unique_ptr<ParseNode> SimplifyExpression(std::unique_ptr<ParseNode> expr); + // Represents a set of patterns within a build file. class LabelMatcher { public:
diff --git a/src/gn/command_suggest_unittest.cc b/src/gn/command_suggest_unittest.cc index ffc6dc6..7998b0b 100644 --- a/src/gn/command_suggest_unittest.cc +++ b/src/gn/command_suggest_unittest.cc
@@ -708,7 +708,6 @@ check_includes_strict = true public = [ "includer.h" ] sources = [ "includer.cc" ] - deps = [] public_deps = [ "//included" ] } )";
diff --git a/src/gn/edit_command_unittest.cc b/src/gn/edit_command_unittest.cc index dd426f7..833af41 100644 --- a/src/gn/edit_command_unittest.cc +++ b/src/gn/edit_command_unittest.cc
@@ -293,6 +293,18 @@ {Err(Location(), "Target \"//:foo\" does not contain the value " "\"//nonexistent\" in attribute \"deps\".")}})); + + EXPECT_SUCCESS(DoEdit("move deps public_deps //a", + R"( +executable("foo") { + deps = [ "//a" ] +} +)"), + Edited(R"( +executable("foo") { + public_deps = [ "//a" ] +} +)")); } TEST_F(EditCommandTest, NewSubcommand) { @@ -459,7 +471,7 @@ )"), Edited(R"( executable("foo") { - deps = [] + [ "//foo:bar" ] + deps = [ "//foo:bar" ] } )")); @@ -479,6 +491,93 @@ "Target \"//:foo\" does not contain the " "value \"//nonexistent\" in attribute " "\"deps\".")}})); + + EXPECT_SUCCESS(DoEdit("remove deps //base", + R"( +executable("foo") { + deps = [ "//base" ] +} +)"), + Edited(R"( +executable("foo") { +} +)")); + + EXPECT_SUCCESS(DoEdit("remove deps //base", + R"( +executable("foo") { + deps = [ "//base" ] + if (is_linux) { + deps += [ "//linux" ] + } +} +)"), + Edited(R"( +executable("foo") { + if (is_linux) { + deps = [ "//linux" ] + } +} +)")); + + EXPECT_SUCCESS(DoEdit("remove deps //a", + R"( +executable("foo") { + deps = [ "//a" ] + deps += [ "//b" ] +} +)"), + Edited(R"( +executable("foo") { + deps = [ "//b" ] +} +)")); + + EXPECT_SUCCESS(DoEdit("remove deps //base", + R"( +executable("foo") { + deps = [ "//base" ] + if (is_linux) { + deps += [ "//linux" ] + } + deps += [ "//other" ] +} +)"), + Edited(R"( +executable("foo") { + deps = [] + if (is_linux) { + deps += [ "//linux" ] + } + deps += [ "//other" ] +} +)")); + + // When multiple assignments exist, an empty assignment must not be removed, + // and subsequent conditional += must not be converted to =. + EXPECT_SUCCESS(DoEdit("remove deps //base", + R"( +executable("foo") { + deps = [ "//base" ] + if (is_linux) { + deps += [ "//linux" ] + } + if (is_mac) { + deps += [ "//mac" ] + } +} +)"), + Edited(R"( +executable("foo") { + deps = [] + if (is_linux) { + deps += [ "//linux" ] + } + if (is_mac) { + deps += [ "//mac" ] + } +} +)")); } TEST_F(EditCommandTest, RenameSubcommand) {
diff --git a/src/gn/edit_subcommands.cc b/src/gn/edit_subcommands.cc index dbcc051..dbc6b67 100644 --- a/src/gn/edit_subcommands.cc +++ b/src/gn/edit_subcommands.cc
@@ -128,7 +128,35 @@ done |= !matches.empty(); } - if (!done && target.is_explicit) { + if (done) { + auto assignments = target.assignments(attribute); + for (auto i = 0u; i < assignments.size(); ++i) { + auto& assign = assignments[i]; + auto* op = assignments[i]->AsBinaryOpMut(); + CHECK(op); + TreeNode* next = + i + 1 < assignments.size() ? &assignments[i + 1] : nullptr; + + op->set_right(SimplifyExpression(op->take_right())); + if (IsEmptyList(op->right())) { + if (assign.is_modification() || assignments.size() == 1) { + assign.RemoveSelfUnconditionally(); + // Transform a = []; a += ["..."] => a = ["..."] + // This can be safely done if `a = []` is unconditional, and either + // the += is unconditional, or we know it's the last assignment. + } else if (next && !assign.is_conditional() && + (!next->is_conditional() || assignments.size() == 2)) { + auto* next_op = next->node()->AsBinaryOpMut(); + if (next_op->op().type() == Token::PLUS_EQUALS) { + next_op->set_op(Token(next_op->op().location(), Token::EQUAL, "=")); + assign.RemoveSelfUnconditionally(); + } else if (next_op->op().type() == Token::EQUAL) { + assign.RemoveSelfUnconditionally(); + } + } + } + } + } else if (target.is_explicit) { target.add_warning(state, "does not contain the value " + value.ToString(true) + " in attribute \"" + attribute + "\".");