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());
+}
