Remove some appending behavior from GN. Remove operator += and -= to append/remove a non-list to/from a list. Now you will have to wrap a single item in [] if you want to add or remove it. This makes the syntax more uniform since people were doing it randomly with and without []. This removes the error of assigning to an unused variable. This ended up being too much of a bother, and causing errors in cases were you wouldn't expect it. R=scottmg@chromium.org Review URL: https://codereview.chromium.org/136723008 Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: ff32a64e062e9a61047445f971167700dfb0a481
diff --git a/tools/gn/operators.cc b/tools/gn/operators.cc index 9418013..5824ac3 100644 --- a/tools/gn/operators.cc +++ b/tools/gn/operators.cc
@@ -96,6 +96,8 @@ // Assignment ----------------------------------------------------------------- +// We return a null value from this rather than the result of doing the append. +// See ValuePlusEquals for rationale. Value ExecuteEquals(Scope* scope, const BinaryOpNode* op_node, const Token& left, @@ -103,44 +105,27 @@ Err* err) { const Value* old_value = scope->GetValue(left.value(), false); if (old_value) { - if (scope->IsSetButUnused(left.value())) { - // Throw an error for re-assigning without using the value first. The - // exception is that you can overwrite an empty list with another list - // since this is the way to get around the "can't overwrite a nonempty - // list with another nonempty list" restriction. - if (old_value->type() != Value::LIST || - !old_value->list_value().empty()) { - *err = Err(op_node->left()->GetRange(), "Overwriting unused variable.", - "This overwrites a previous assignment to \"" + - left.value().as_string() + "\" that had no effect."); - err->AppendSubErr(Err(*scope->GetValue(left.value()), - "Previously set here.", - "Maybe you wanted \"+=\" to append instead?")); - return Value(); - } - } else { - // Throw an error when overwriting a nonempty list with another nonempty - // list item. This is to detect the case where you write - // defines = ["FOO"] - // and you overwrote inherited ones, when instead you mean to append: - // defines += ["FOO"] - if (old_value->type() == Value::LIST && - !old_value->list_value().empty() && - right.type() == Value::LIST && - !right.list_value().empty()) { - *err = Err(op_node->left()->GetRange(), "Replacing nonempty list.", - std::string("This overwrites a previously-defined nonempty list ") + - "(length " + - base::IntToString(static_cast<int>(old_value->list_value().size())) - + ")."); - err->AppendSubErr(Err(*old_value, "for previous definition", - "with another one (length " + - base::IntToString(static_cast<int>(right.list_value().size())) + - "). Did you mean " + - "\"+=\" to append instead? If you\nreally want to do this, do\n " + - left.value().as_string() + " = []\nbefore reassigning.")); - return Value(); - } + // Throw an error when overwriting a nonempty list with another nonempty + // list item. This is to detect the case where you write + // defines = ["FOO"] + // and you overwrote inherited ones, when instead you mean to append: + // defines += ["FOO"] + if (old_value->type() == Value::LIST && + !old_value->list_value().empty() && + right.type() == Value::LIST && + !right.list_value().empty()) { + *err = Err(op_node->left()->GetRange(), "Replacing nonempty list.", + std::string("This overwrites a previously-defined nonempty list ") + + "(length " + + base::IntToString(static_cast<int>(old_value->list_value().size())) + + ")."); + err->AppendSubErr(Err(*old_value, "for previous definition", + "with another one (length " + + base::IntToString(static_cast<int>(right.list_value().size())) + + "). Did you mean " + + "\"+=\" to append instead? If you\nreally want to do this, do\n " + + left.value().as_string() + " = []\nbefore reassigning.")); + return Value(); } } if (err->has_error()) @@ -163,6 +148,14 @@ // allow_type_conversion indicates if we're allowed to change the type of the // left value. This is set to true when doing +, and false when doing +=. +// +// Note that we return Value() from here, which is different than C++. This +// means you can't do clever things like foo = [ bar += baz ] to simultaneously +// append to and use a value. This is basically never needed in out build +// scripts and is just as likely an error as the intended behavior, and it also +// involves a copy of the value when it's returned. Many times we're appending +// to large lists, and copying the value to discard it for the next statement +// is very wasteful. void ValuePlusEquals(const Scope* scope, const BinaryOpNode* op_node, const Token& left_token, @@ -210,14 +203,6 @@ // Left-hand-side list. case Value::LIST: switch (right.type()) { - case Value::INTEGER: // list + integer -> list append. - case Value::STRING: // list + string -> list append. - if (left_token.value() == kSourcesName) - AppendFilteredSourcesToValue(scope, right, left); - else - left->list_value().push_back(right); - return; - case Value::LIST: // list + list -> list concat. if (left_token.value() == kSourcesName) { // Filter additions through the assignment filter. @@ -230,7 +215,9 @@ return; default: - break; + *err = Err(op_node->op(), "Incompatible types to add.", + "To append a single item to a list do \"foo += [ bar ]\"."); + return; } default: @@ -262,6 +249,8 @@ return Value(); } +// We return a null value from this rather than the result of doing the append. +// See ValuePlusEquals for rationale. void ValueMinusEquals(const BinaryOpNode* op_node, Value* left, const Value& right, @@ -286,7 +275,12 @@ // Left-hand-side list. case Value::LIST: - RemoveMatchesFromList(op_node, left, right, err); + if (right.type() != Value::LIST) { + *err = Err(op_node->op(), "Incompatible types to subtract.", + "To remove a single item from a list do \"foo -= [ bar ]\"."); + } else { + RemoveMatchesFromList(op_node, left, right, err); + } return; default:
diff --git a/tools/gn/operators_unittest.cc b/tools/gn/operators_unittest.cc index 16d626f..529c883 100644 --- a/tools/gn/operators_unittest.cc +++ b/tools/gn/operators_unittest.cc
@@ -22,6 +22,15 @@ return v.string_value() == s; } +// Returns a list populated with a single literal Value corresponding to the +// given token. The token must outlive the list (since the list will just +// copy the reference). +scoped_ptr<ListNode> ListWithLiteral(const Token& token) { + scoped_ptr<ListNode> list(new ListNode); + list->append_item(scoped_ptr<ParseNode>(new LiteralNode(token))); + return list.Pass(); +} + } // namespace TEST(Operators, SourcesAppend) { @@ -50,21 +59,21 @@ // Append an integer. const char integer_value[] = "5"; Token integer(Location(), Token::INTEGER, integer_value); - node.set_right(scoped_ptr<ParseNode>(new LiteralNode(integer))); + node.set_right(ListWithLiteral(integer).PassAs<ParseNode>()); node.Execute(setup.scope(), &err); EXPECT_FALSE(err.has_error()); // Append a string that doesn't match the pattern, it should get appended. const char string_1_value[] = "\"good\""; Token string_1(Location(), Token::STRING, string_1_value); - node.set_right(scoped_ptr<ParseNode>(new LiteralNode(string_1))); + node.set_right(ListWithLiteral(string_1).PassAs<ParseNode>()); node.Execute(setup.scope(), &err); EXPECT_FALSE(err.has_error()); // Append a string that does match the pattern, it should be a no-op. const char string_2_value[] = "\"foo-rm\""; Token string_2(Location(), Token::STRING, string_2_value); - node.set_right(scoped_ptr<ParseNode>(new LiteralNode(string_2))); + node.set_right(ListWithLiteral(string_2).PassAs<ParseNode>()); node.Execute(setup.scope(), &err); EXPECT_FALSE(err.has_error()); @@ -84,3 +93,61 @@ EXPECT_TRUE(IsValueStringEqualing(value->list_value()[1], "good")); EXPECT_TRUE(IsValueStringEqualing(value->list_value()[2], "good")); } + +// Note that the SourcesAppend test above tests the basic list + list features, +// this test handles the other cases. +TEST(Operators, ListAppend) { + Err err; + TestWithScope setup; + + // Set up "foo" with an empty list. + const char foo[] = "foo"; + setup.scope()->SetValue(foo, Value(NULL, Value::LIST), NULL); + + // Set up the operator. + BinaryOpNode node; + const char token_value[] = "+="; + Token op(Location(), Token::PLUS_EQUALS, token_value); + node.set_op(op); + + // Append to the foo variable. + Token identifier_token(Location(), Token::IDENTIFIER, foo); + node.set_left(scoped_ptr<ParseNode>(new IdentifierNode(identifier_token))); + + // Append a list with a list, the result should be a nested list. + scoped_ptr<ListNode> outer_list(new ListNode); + const char twelve_str[] = "12"; + Token twelve(Location(), Token::INTEGER, twelve_str); + outer_list->append_item(ListWithLiteral(twelve).PassAs<ParseNode>()); + node.set_right(outer_list.PassAs<ParseNode>()); + + Value ret = ExecuteBinaryOperator(setup.scope(), &node, node.left(), + node.right(), &err); + EXPECT_FALSE(err.has_error()); + + // Return from the operator should always be "none", it should update the + // value only. + EXPECT_EQ(Value::NONE, ret.type()); + + // The value should be updated with "[ [ 12 ] ]" + Value result = *setup.scope()->GetValue(foo); + ASSERT_EQ(Value::LIST, result.type()); + ASSERT_EQ(1u, result.list_value().size()); + ASSERT_EQ(Value::LIST, result.list_value()[0].type()); + ASSERT_EQ(1u, result.list_value()[0].list_value().size()); + ASSERT_EQ(Value::INTEGER, result.list_value()[0].list_value()[0].type()); + ASSERT_EQ(12, result.list_value()[0].list_value()[0].int_value()); + + // Try to append an integer and a string directly (e.g. foo += "hi"). + // This should fail. + const char str_str[] = "\"hi\""; + Token str(Location(), Token::STRING, str_str); + node.set_right(scoped_ptr<ParseNode>(new LiteralNode(str))); + ExecuteBinaryOperator(setup.scope(), &node, node.left(), node.right(), &err); + EXPECT_TRUE(err.has_error()); + err = Err(); + + node.set_right(scoped_ptr<ParseNode>(new LiteralNode(twelve))); + ExecuteBinaryOperator(setup.scope(), &node, node.left(), node.right(), &err); + EXPECT_TRUE(err.has_error()); +}