Allow creation and modification of scopes in GN.
Previously scope variables could only be created by exec_script() and read_file() functions. This patch adds the following syntax:
foo = { a = 1 b = 2 }
foo.a += 1
foo.c = 3
This will be used in a followup for toolchain args for non-default toolchains, which will allow us to generalize the templates as the number of configurations grows over time (currently every variable has be to be forwarded in the root toolchain templates).
It also allows mutations of list elements which isn't actually necessary for this but the implementation is the same as for scope mutations:
list[0] = "foo"
This does a significant rework of the operator implementation. This was required for the more general "destination" for assignments and mutations.
It also updates the operator functions to use move semantics more. Previously:
list3 = list1 + list2
Would be implemented as:
1. Copy list1 into new new variable.
2. Copy list2 into a new variable.
3. Copy list1's copy to a new variable.
4. Append elements of list2 to list1's copy by copying.
5. Copy that list to list3.
That's a lot of copies! The new implementation does:
1. Copy list1 into a new variable.
2. Copy list2 into a new variable.
3. Append elements of list2's copy to list1's copy by moving.
4. Move list2 to list3.
Assuming the lists contain strings, each string should now be copied exactly once rather than three times.
Added a lot of documentation to the grammar help on how the language works (not strictly grammar but it seems like the best place to put it.
BUG=
Review-Url: https://codereview.chromium.org/2187523003
Cr-Original-Commit-Position: refs/heads/master@{#409672}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 01cdea1ed9ad32a7a595ef1a5adfa384006e3097
diff --git a/tools/gn/command_help.cc b/tools/gn/command_help.cc
index 9bac894..68f0d04 100644
--- a/tools/gn/command_help.cc
+++ b/tools/gn/command_help.cc
@@ -62,7 +62,7 @@
PrintShortHelp("all: Print all the help at once");
PrintShortHelp("buildargs: How build arguments work.");
PrintShortHelp("dotfile: Info about the toplevel .gn file.");
- PrintShortHelp("grammar: Formal grammar for GN build files.");
+ PrintShortHelp("grammar: Language and grammar for GN build files.");
PrintShortHelp(
"input_conversion: Processing input from exec_script and read_file.");
PrintShortHelp("label_pattern: Matching more than one label.");
diff --git a/tools/gn/function_foreach.cc b/tools/gn/function_foreach.cc
index d8fe87e..c264e3c 100644
--- a/tools/gn/function_foreach.cc
+++ b/tools/gn/function_foreach.cc
@@ -95,7 +95,8 @@
if (old_loop_value_ptr) {
// Put back old value. Use the copy we made, rather than use the pointer,
// which will probably point to the new value now in the scope.
- scope->SetValue(loop_var, old_loop_value, old_loop_value.origin());
+ scope->SetValue(loop_var, std::move(old_loop_value),
+ old_loop_value.origin());
} else {
// Loop variable was undefined before loop, delete it.
scope->RemoveIdentifier(loop_var);
diff --git a/tools/gn/function_forward_variables_from.cc b/tools/gn/function_forward_variables_from.cc
index 8f3b90d..8c7a558 100644
--- a/tools/gn/function_forward_variables_from.cc
+++ b/tools/gn/function_forward_variables_from.cc
@@ -183,7 +183,8 @@
}
// Extract the source scope.
- Value* value = scope->GetMutableValue(identifier->value().value(), true);
+ Value* value = scope->GetMutableValue(
+ identifier->value().value(), Scope::SEARCH_NESTED, true);
if (!value) {
*err = Err(identifier, "Undefined identifier.");
return Value();
diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc
index dcea722..02129a8 100644
--- a/tools/gn/functions.cc
+++ b/tools/gn/functions.cc
@@ -612,10 +612,12 @@
"\n"
" The sources assignment filter is a list of patterns that remove files\n"
" from the list implicitly whenever the \"sources\" variable is\n"
- " assigned to. This is intended to be used to globally filter out files\n"
- " with platform-specific naming schemes when they don't apply, for\n"
- " example, you may want to filter out all \"*_win.cc\" files on non-\n"
- " Windows platforms.\n"
+ " assigned to. This will do nothing for non-lists.\n"
+ "\n"
+ " This is intended to be used to globally filter out files with\n"
+ " platform-specific naming schemes when they don't apply, for example\n"
+ " you may want to filter out all \"*_win.cc\" files on non-Windows\n"
+ " platforms.\n"
"\n"
" Typically this will be called once in the master build config script\n"
" to set up the filter for the current platform. Subsequent calls will\n"
diff --git a/tools/gn/loader_unittest.cc b/tools/gn/loader_unittest.cc
index c6cdbdb..20b8609 100644
--- a/tools/gn/loader_unittest.cc
+++ b/tools/gn/loader_unittest.cc
@@ -96,7 +96,7 @@
}
void MockInputFileManager::IssueAllPending() {
- BlockNode block; // Default response.
+ BlockNode block(BlockNode::DISCARDS_RESULT); // Default response.
for (const auto& cur : pending_) {
CannedResponseMap::const_iterator found = canned_responses_.find(cur.first);
diff --git a/tools/gn/operators.cc b/tools/gn/operators.cc
index 6b3f7c7..ac6a37f 100644
--- a/tools/gn/operators.cc
+++ b/tools/gn/operators.cc
@@ -5,6 +5,7 @@
#include "tools/gn/operators.h"
#include <stddef.h>
+#include <algorithm>
#include "base/strings/string_number_conversions.h"
#include "tools/gn/err.h"
@@ -17,41 +18,212 @@
const char kSourcesName[] = "sources";
-// Applies the sources assignment filter from the given scope to each element
-// of source (can be a list or a string), appending it to dest if it doesn't
-// match.
-void AppendFilteredSourcesToValue(const Scope* scope,
- const Value& source,
- Value* dest) {
- const PatternList* filter = scope->GetSourcesAssignmentFilter();
+// Helper class used for assignment operations: =, +=, and -= to generalize
+// writing to various types of destinations.
+class ValueDestination {
+ public:
+ ValueDestination();
- if (source.type() == Value::STRING) {
- if (!filter || filter->is_empty() ||
- !filter->MatchesValue(source))
- dest->list_value().push_back(source);
- return;
- }
- if (source.type() != Value::LIST) {
- // Any non-list and non-string being added to a list can just get appended,
- // we're not going to filter it.
- dest->list_value().push_back(source);
- return;
+ bool Init(Scope* exec_scope,
+ const ParseNode* dest,
+ const BinaryOpNode* op_node,
+ Err* err);
+
+ // Returns the value in the destination scope if it already exists, or null
+ // if it doesn't. This is for validation and does not count as a "use".
+ // Other nested scopes will be searched.
+ const Value* GetExistingValue() const;
+
+ // Returns an existing version of the output if it can be modified. This will
+ // not search nested scopes since writes only go into the current scope.
+ // Returns null if the value does not exist, or is not in the current scope
+ // (meaning assignments won't go to this value and it's not mutable). This
+ // is for implementing += and -=.
+ //
+ // If it exists, this will mark the origin of the value to be the passed-in
+ // node, and the value will be also marked unused (if possible) under the
+ // assumption that it will be modified in-place.
+ Value* GetExistingMutableValueIfExists(const ParseNode* origin);
+
+ // Returns the sources assignment filter if it exists for the current
+ // scope and it should be applied to this assignment. Otherwise returns null.
+ const PatternList* GetAssignmentFilter(const Scope* exec_scope) const;
+
+ // Returns a pointer to the value that was set.
+ Value* SetValue(Value value, const ParseNode* set_node);
+
+ // Fills the Err with an undefined value error appropriate for modification
+ // operators: += and -= (where the source is also the dest).
+ void MakeUndefinedIdentifierForModifyError(Err* err);
+
+ private:
+ enum Type { UNINITIALIZED, SCOPE, LIST };
+
+ Type type_;
+
+ // Valid when type_ == SCOPE.
+ Scope* scope_;
+ const Token* name_token_;
+
+ // Valid when type_ == LIST.
+ Value* list_;
+ size_t index_; // Guaranteed in-range when Init() succeeds.
+};
+
+ValueDestination::ValueDestination()
+ : type_(UNINITIALIZED),
+ scope_(nullptr),
+ name_token_(nullptr),
+ list_(nullptr),
+ index_(0) {
+}
+
+bool ValueDestination::Init(Scope* exec_scope,
+ const ParseNode* dest,
+ const BinaryOpNode* op_node,
+ Err* err) {
+ // Check for standard variable set.
+ const IdentifierNode* dest_identifier = dest->AsIdentifier();
+ if (dest_identifier) {
+ type_ = SCOPE;
+ scope_ = exec_scope;
+ name_token_ = &dest_identifier->value();
+ return true;
}
- if (!filter || filter->is_empty()) {
- // No filter, append everything.
- for (const auto& source_entry : source.list_value())
- dest->list_value().push_back(source_entry);
- return;
+ // Check for array and scope accesses. The base (array or scope variable
+ // name) must always be defined ahead of time.
+ const AccessorNode* dest_accessor = dest->AsAccessor();
+ if (!dest_accessor) {
+ *err = Err(op_node, "Assignment requires a lvalue.",
+ "This thing on the left is not an identifier or accessor.");
+ err->AppendRange(dest->GetRange());
+ return false;
}
- // Note: don't reserve() the dest vector here since that actually hurts
- // the allocation pattern when the build script is doing multiple small
- // additions.
- for (const auto& source_entry : source.list_value()) {
- if (!filter->MatchesValue(source_entry))
- dest->list_value().push_back(source_entry);
+ // Known to be an accessor.
+ base::StringPiece base_str = dest_accessor->base().value();
+ Value* base = exec_scope->GetMutableValue(
+ base_str, Scope::SEARCH_CURRENT, false);
+ if (!base) {
+ // Base is either undefined or it's defined but not in the current scope.
+ // Make a good error message.
+ if (exec_scope->GetValue(base_str, false)) {
+ *err = Err(dest_accessor->base(), "Suspicious in-place modification.",
+ "This variable exists in a containing scope. Normally, writing to it "
+ "would\nmake a copy of it into the current scope with the modified "
+ "version. But\nhere you're modifying only an element of a scope or "
+ "list object. It's unlikely\nyou meant to copy the entire thing just "
+ "to modify this part of it.\n"
+ "\n"
+ "If you really wanted to do this, do:\n"
+ " " + base_str.as_string() + " = " + base_str.as_string() + "\n"
+ "to copy it into the current scope before doing this operation.");
+ } else {
+ *err = Err(dest_accessor->base(), "Undefined identifier.");
+ }
+ return false;
}
+
+ if (dest_accessor->index()) {
+ // List access with an index.
+ if (!base->VerifyTypeIs(Value::LIST, err)) {
+ // Errors here will confusingly refer to the variable declaration (since
+ // that's all Value knows) rather than the list access. So rewrite the
+ // error location to refer to the base value's location.
+ *err = Err(dest_accessor->base(), err->message(), err->help_text());
+ return false;
+ }
+
+ type_ = LIST;
+ list_ = base;
+ return dest_accessor->ComputeAndValidateListIndex(
+ exec_scope, base->list_value().size(), &index_, err);
+ }
+
+ // Scope access with a dot.
+ if (!base->VerifyTypeIs(Value::SCOPE, err)) {
+ // As the for the list index case above, rewrite the error location.
+ *err = Err(dest_accessor->base(), err->message(), err->help_text());
+ return false;
+ }
+ type_ = SCOPE;
+ scope_ = base->scope_value();
+ name_token_ = &dest_accessor->member()->value();
+ return true;
+}
+
+const Value* ValueDestination::GetExistingValue() const {
+ if (type_ == SCOPE)
+ return scope_->GetValue(name_token_->value(), false);
+ else if (type_ == LIST)
+ return &list_->list_value()[index_];
+ return nullptr;
+}
+
+Value* ValueDestination::GetExistingMutableValueIfExists(
+ const ParseNode* origin) {
+ if (type_ == SCOPE) {
+ Value* value = scope_->GetMutableValue(
+ name_token_->value(), Scope::SEARCH_CURRENT, false);
+ if (value) {
+ // The value will be written to, reset its tracking information.
+ value->set_origin(origin);
+ scope_->MarkUnused(name_token_->value());
+ }
+ }
+ if (type_ == LIST)
+ return &list_->list_value()[index_];
+ return nullptr;
+}
+
+const PatternList* ValueDestination::GetAssignmentFilter(
+ const Scope* exec_scope) const {
+ if (type_ != SCOPE)
+ return nullptr; // Destination can't be named, so no sources filtering.
+ if (name_token_->value() != kSourcesName)
+ return nullptr; // Destination not named "sources".
+
+ const PatternList* filter = exec_scope->GetSourcesAssignmentFilter();
+ if (!filter || filter->is_empty())
+ return nullptr; // No filter or empty filter, don't need to do anything.
+ return filter;
+}
+
+Value* ValueDestination::SetValue(Value value, const ParseNode* set_node) {
+ if (type_ == SCOPE) {
+ return scope_->SetValue(name_token_->value(), std::move(value), set_node);
+ } else if (type_ == LIST) {
+ Value* dest = &list_->list_value()[index_];
+ *dest = std::move(value);
+ return dest;
+ }
+ return nullptr;
+}
+
+void ValueDestination::MakeUndefinedIdentifierForModifyError(Err* err) {
+ // When Init() succeeds, the base of any accessor has already been resolved
+ // and that list indices are in-range. This means any undefined identifiers
+ // are for scope accesses.
+ DCHECK(type_ == SCOPE);
+ *err = Err(*name_token_, "Undefined identifier.");
+}
+
+// -----------------------------------------------------------------------------
+
+Err MakeIncompatibleTypeError(const BinaryOpNode* op_node,
+ const Value& left,
+ const Value& right) {
+ std::string msg =
+ std::string("You can't do <") + Value::DescribeType(left.type()) + "> " +
+ op_node->op().value().as_string() +
+ " <" + Value::DescribeType(right.type()) + ">.";
+ if (left.type() == Value::LIST) {
+ // Append extra hint for list stuff.
+ msg += "\n\nHint: If you're attempting to add or remove a single item from "
+ " a list, use \"foo + [ bar ]\".";
+ }
+ return Err(op_node, "Incompatible types for binary operator.", msg);
}
Value GetValueOrFillError(const BinaryOpNode* op_node,
@@ -118,12 +290,12 @@
// We return a null value from this rather than the result of doing the append.
// See ValuePlusEquals for rationale.
-Value ExecuteEquals(Scope* scope,
+Value ExecuteEquals(Scope* exec_scope,
const BinaryOpNode* op_node,
- const Token& left,
- const Value& right,
+ ValueDestination* dest,
+ Value right,
Err* err) {
- const Value* old_value = scope->GetValue(left.value(), false);
+ const Value* old_value = dest->GetExistingValue();
if (old_value) {
// Throw an error when overwriting a nonempty list with another nonempty
// list item. This is to detect the case where you write
@@ -143,215 +315,216 @@
"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."));
+ "\"+=\" to append instead? If you\nreally want to do this, do\n"
+ " foo = []\nbefore reassigning."));
return Value();
}
}
if (err->has_error())
return Value();
- if (right.type() == Value::LIST && left.value() == kSourcesName) {
- // Assigning to sources, filter the list. Here we do the filtering and
- // copying in one step to save an extra list copy (the lists may be
- // long).
- Value* set_value = scope->SetValue(left.value(),
- Value(op_node, Value::LIST), op_node);
- set_value->list_value().reserve(right.list_value().size());
- AppendFilteredSourcesToValue(scope, right, set_value);
- } else {
- // Normal value set, just copy it.
- scope->SetValue(left.value(), right, op_node->right());
+ Value* written_value = dest->SetValue(std::move(right), op_node->right());
+
+ // Optionally apply the assignment filter in-place.
+ const PatternList* filter = dest->GetAssignmentFilter(exec_scope);
+ if (filter) {
+ std::vector<Value>& list_value = written_value->list_value();
+ auto first_deleted = std::remove_if(
+ list_value.begin(), list_value.end(),
+ [filter](const Value& v) {
+ return filter->MatchesValue(v);
+ });
+ list_value.erase(first_deleted, list_value.end());
}
return Value();
}
-// 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,
- Value* left,
- const Value& right,
- bool allow_type_conversion,
- Err* err) {
- switch (left->type()) {
- // Left-hand-side int.
- case Value::INTEGER:
- switch (right.type()) {
- case Value::INTEGER: // int + int -> addition.
- left->int_value() += right.int_value();
- return;
+// Plus/minus ------------------------------------------------------------------
- case Value::STRING: // int + string -> string concat.
- if (allow_type_conversion) {
- *left = Value(op_node,
- base::Int64ToString(left->int_value()) + right.string_value());
- return;
- }
- break;
-
- default:
- break;
- }
- break;
-
- // Left-hand-side string.
- case Value::STRING:
- switch (right.type()) {
- case Value::INTEGER: // string + int -> string concat.
- left->string_value().append(base::Int64ToString(right.int_value()));
- return;
-
- case Value::STRING: // string + string -> string contat.
- left->string_value().append(right.string_value());
- return;
-
- default:
- break;
- }
- break;
-
- // Left-hand-side list.
- case Value::LIST:
- switch (right.type()) {
- case Value::LIST: // list + list -> list concat.
- if (left_token.value() == kSourcesName) {
- // Filter additions through the assignment filter.
- AppendFilteredSourcesToValue(scope, right, left);
- } else {
- // Normal list concat.
- for (const auto& value : right.list_value())
- left->list_value().push_back(value);
- }
- return;
-
- default:
- *err = Err(op_node->op(), "Incompatible types to add.",
- "To append a single item to a list do \"foo += [ bar ]\".");
- return;
- }
-
- default:
- break;
- }
-
- *err = Err(op_node->op(), "Incompatible types to add.",
- std::string("I see a ") + Value::DescribeType(left->type()) + " and a " +
- Value::DescribeType(right.type()) + ".");
-}
-
-Value ExecutePlusEquals(Scope* scope,
- const BinaryOpNode* op_node,
- const Token& left,
- const Value& right,
- Err* err) {
- // We modify in-place rather than doing read-modify-write to avoid
- // copying large lists.
- Value* left_value =
- scope->GetValueForcedToCurrentScope(left.value(), op_node);
- if (!left_value) {
- *err = Err(left, "Undefined variable for +=.",
- "I don't have something with this name in scope now.");
- return Value();
- }
- ValuePlusEquals(scope, op_node, left, left_value, right, false, err);
- left_value->set_origin(op_node);
- scope->MarkUnused(left.value());
- 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,
- bool allow_type_conversion,
- Err* err) {
- switch (left->type()) {
- // Left-hand-side int.
- case Value::INTEGER:
- switch (right.type()) {
- case Value::INTEGER: // int - int -> subtraction.
- left->int_value() -= right.int_value();
- return;
-
- default:
- break;
- }
- break;
-
- // Left-hand-side string.
- case Value::STRING:
- break; // All are errors.
-
- // Left-hand-side list.
- case Value::LIST:
- 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:
- break;
- }
-
- *err = Err(op_node->op(), "Incompatible types to subtract.",
- std::string("I see a ") + Value::DescribeType(left->type()) + " and a " +
- Value::DescribeType(right.type()) + ".");
-}
-
-Value ExecuteMinusEquals(Scope* scope,
- const BinaryOpNode* op_node,
- const Token& left,
- const Value& right,
- Err* err) {
- Value* left_value =
- scope->GetValueForcedToCurrentScope(left.value(), op_node);
- if (!left_value) {
- *err = Err(left, "Undefined variable for -=.",
- "I don't have something with this name in scope now.");
- return Value();
- }
- ValueMinusEquals(op_node, left_value, right, false, err);
- left_value->set_origin(op_node);
- scope->MarkUnused(left.value());
- return Value();
-}
-
-// Plus/Minus -----------------------------------------------------------------
-
-Value ExecutePlus(Scope* scope,
- const BinaryOpNode* op_node,
- const Value& left,
- const Value& right,
+// allow_left_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 +=.
+Value ExecutePlus(const BinaryOpNode* op_node,
+ Value left,
+ Value right,
+ bool allow_left_type_conversion,
Err* err) {
- Value ret = left;
- ValuePlusEquals(scope, op_node, Token(), &ret, right, true, err);
- ret.set_origin(op_node);
- return ret;
+ // Left-hand-side integer.
+ if (left.type() == Value::INTEGER) {
+ if (right.type() == Value::INTEGER) {
+ // Int + int -> addition.
+ return Value(op_node, left.int_value() + right.int_value());
+ } else if (right.type() == Value::STRING && allow_left_type_conversion) {
+ // Int + string -> string concat.
+ return Value(
+ op_node,
+ base::Int64ToString(left.int_value()) + right.string_value());
+ }
+ *err = MakeIncompatibleTypeError(op_node, left, right);
+ return Value();
+ }
+
+ // Left-hand-side string.
+ if (left.type() == Value::STRING) {
+ if (right.type() == Value::INTEGER) {
+ // String + int -> string concat.
+ return Value(op_node,
+ left.string_value() + base::Int64ToString(right.int_value()));
+ } else if (right.type() == Value::STRING) {
+ // String + string -> string concat. Since the left is passed by copy
+ // we can avoid realloc if there is enough buffer by appending to left
+ // and assigning.
+ left.string_value().append(right.string_value());
+ return left; // FIXME(brettw) des this copy?
+ }
+ *err = MakeIncompatibleTypeError(op_node, left, right);
+ return Value();
+ }
+
+ // Left-hand-side list. The only valid thing is to add another list.
+ if (left.type() == Value::LIST && right.type() == Value::LIST) {
+ // Since left was passed by copy, avoid realloc by destructively appending
+ // to it and using that as the result.
+ for (Value& value : right.list_value())
+ left.list_value().push_back(std::move(value));
+ return left; // FIXME(brettw) does this copy?
+ }
+
+ *err = MakeIncompatibleTypeError(op_node, left, right);
+ return Value();
}
-Value ExecuteMinus(Scope* scope,
- const BinaryOpNode* op_node,
- const Value& left,
+// Left is passed by value because it will be modified in-place and returned
+// for the list case.
+Value ExecuteMinus(const BinaryOpNode* op_node,
+ Value left,
const Value& right,
Err* err) {
- Value ret = left;
- ValueMinusEquals(op_node, &ret, right, true, err);
- ret.set_origin(op_node);
- return ret;
+ // Left-hand-side int. The only thing to do is subtract another int.
+ if (left.type() == Value::INTEGER && right.type() != Value::INTEGER) {
+ // Int - int -> subtraction.
+ return Value(op_node, left.int_value() - right.int_value());
+ }
+
+ // Left-hand-side list. The only thing to do is subtract another list.
+ if (left.type() == Value::LIST && right.type() == Value::LIST) {
+ // In-place modify left and return it.
+ RemoveMatchesFromList(op_node, &left, right, err);
+ return left;
+ }
+
+ *err = MakeIncompatibleTypeError(op_node, left, right);
+ return Value();
+}
+
+// In-place plus/minus ---------------------------------------------------------
+
+void ExecutePlusEquals(Scope* exec_scope,
+ const BinaryOpNode* op_node,
+ ValueDestination* dest,
+ Value right,
+ Err* err) {
+ // There are several cases. Some things we can convert "foo += bar" to
+ // "foo = foo + bar". Some cases we can't (the 'sources' variable won't
+ // get the right filtering on the list). Some cases we don't want to (lists
+ // and strings will get unnecessary copying so we can to optimize these).
+ //
+ // - Value is already mutable in the current scope:
+ // 1. List/string append: use it.
+ // 2. Other types: fall back to "foo = foo + bar"
+ //
+ // - Value is not mutable in the current scope:
+ // 3. List/string append: copy into current scope and append to that.
+ // 4. Other types: fall back to "foo = foo + bar"
+ //
+ // The common case is to use += for list and string appends in the local
+ // scope, so this is written to avoid multiple variable lookups in that case.
+ Value* mutable_dest = dest->GetExistingMutableValueIfExists(op_node);
+ if (!mutable_dest) {
+ const Value* existing_value = dest->GetExistingValue();
+ if (!existing_value) {
+ // Undefined left-hand-size for +=.
+ dest->MakeUndefinedIdentifierForModifyError(err);
+ return;
+ }
+
+ if (existing_value->type() != Value::STRING &&
+ existing_value->type() != Value::LIST) {
+ // Case #4 above.
+ dest->SetValue(ExecutePlus(op_node, *existing_value,
+ std::move(right), false, err), op_node);
+ return;
+ }
+
+ // Case #3 above, copy to current scope and fall-through to appending.
+ mutable_dest = dest->SetValue(*existing_value, op_node);
+ } else if (mutable_dest->type() != Value::STRING &&
+ mutable_dest->type() != Value::LIST) {
+ // Case #2 above.
+ dest->SetValue(ExecutePlus(op_node, *mutable_dest,
+ std::move(right), false, err), op_node);
+ return;
+ } // "else" is case #1 above.
+
+ if (mutable_dest->type() == Value::STRING) {
+ if (right.type() == Value::INTEGER) {
+ // String + int -> string concat.
+ mutable_dest->string_value().append(
+ base::Int64ToString(right.int_value()));
+ } else if (right.type() == Value::STRING) {
+ // String + string -> string concat.
+ mutable_dest->string_value().append(right.string_value());
+ } else {
+ *err = MakeIncompatibleTypeError(op_node, *mutable_dest, right);
+ }
+ } else if (mutable_dest->type() == Value::LIST) {
+ // List concat.
+ if (right.type() == Value::LIST) {
+ // Note: don't reserve() the dest vector here since that actually hurts
+ // the allocation pattern when the build script is doing multiple small
+ // additions.
+ const PatternList* filter = dest->GetAssignmentFilter(exec_scope);
+ if (filter) {
+ // Filtered list concat.
+ for (Value& value : right.list_value()) {
+ if (!filter->MatchesValue(value))
+ mutable_dest->list_value().push_back(std::move(value));
+ }
+ } else {
+ // Normal list concat. This is a destructive move.
+ for (Value& value : right.list_value())
+ mutable_dest->list_value().push_back(std::move(value));
+ }
+ } else {
+ *err = Err(op_node->op(), "Incompatible types to add.",
+ "To append a single item to a list do \"foo += [ bar ]\".");
+ }
+ }
+}
+
+void ExecuteMinusEquals(const BinaryOpNode* op_node,
+ ValueDestination* dest,
+ const Value& right,
+ Err* err) {
+ // Like the += case, we can convert "foo -= bar" to "foo = foo - bar". Since
+ // there is no sources filtering, this is always semantically valid. The
+ // only case we don't do it is for lists in the current scope which is the
+ // most common case, and also the one that can be optimized the most by
+ // doing it in-place.
+ Value* mutable_dest = dest->GetExistingMutableValueIfExists(op_node);
+ if (!mutable_dest ||
+ (mutable_dest->type() != Value::LIST || right.type() != Value::LIST)) {
+ const Value* existing_value = dest->GetExistingValue();
+ if (!existing_value) {
+ // Undefined left-hand-size for -=.
+ dest->MakeUndefinedIdentifierForModifyError(err);
+ return;
+ }
+ dest->SetValue(ExecuteMinus(op_node, *existing_value, right, err), op_node);
+ return;
+ }
+
+ // In-place removal of items from "right".
+ RemoveMatchesFromList(op_node, mutable_dest, right, err);
}
// Comparison -----------------------------------------------------------------
@@ -520,15 +693,12 @@
if (op.type() == Token::EQUAL ||
op.type() == Token::PLUS_EQUALS ||
op.type() == Token::MINUS_EQUALS) {
- const IdentifierNode* left_id = left->AsIdentifier();
- if (!left_id) {
- *err = Err(op, "Operator requires a lvalue.",
- "This thing on the left is not an identifier.");
- err->AppendRange(left->GetRange());
+ // Compute the left side.
+ ValueDestination dest;
+ if (!dest.Init(scope, left, op_node, err))
return Value();
- }
- const Token& dest = left_id->value();
+ // Compute the right side.
Value right_value = right->Execute(scope, err);
if (err->has_error())
return Value();
@@ -539,13 +709,17 @@
return Value();
}
- if (op.type() == Token::EQUAL)
- return ExecuteEquals(scope, op_node, dest, right_value, err);
- if (op.type() == Token::PLUS_EQUALS)
- return ExecutePlusEquals(scope, op_node, dest, right_value, err);
- if (op.type() == Token::MINUS_EQUALS)
- return ExecuteMinusEquals(scope, op_node, dest, right_value, err);
- NOTREACHED();
+ // "foo += bar" (same for "-=") is converted to "foo = foo + bar" here, but
+ // we pass the original value of "foo" by pointer to avoid a copy.
+ if (op.type() == Token::EQUAL) {
+ ExecuteEquals(scope, op_node, &dest, std::move(right_value), err);
+ } else if (op.type() == Token::PLUS_EQUALS) {
+ ExecutePlusEquals(scope, op_node, &dest, std::move(right_value), err);
+ } else if (op.type() == Token::MINUS_EQUALS) {
+ ExecuteMinusEquals(op_node, &dest, right_value, err);
+ } else {
+ NOTREACHED();
+ }
return Value();
}
@@ -556,6 +730,7 @@
if (op.type() == Token::BOOLEAN_AND)
return ExecuteAnd(scope, op_node, left, right, err);
+ // Everything else works on the evaluated left and right values.
Value left_value = GetValueOrFillError(op_node, left, "left", scope, err);
if (err->has_error())
return Value();
@@ -565,9 +740,11 @@
// +, -.
if (op.type() == Token::MINUS)
- return ExecuteMinus(scope, op_node, left_value, right_value, err);
- if (op.type() == Token::PLUS)
- return ExecutePlus(scope, op_node, left_value, right_value, err);
+ return ExecuteMinus(op_node, std::move(left_value), right_value, err);
+ if (op.type() == Token::PLUS) {
+ return ExecutePlus(op_node, std::move(left_value), std::move(right_value),
+ true, err);
+ }
// Comparisons.
if (op.type() == Token::EQUAL_EQUAL)
diff --git a/tools/gn/operators_unittest.cc b/tools/gn/operators_unittest.cc
index dd31b82..e2c396c 100644
--- a/tools/gn/operators_unittest.cc
+++ b/tools/gn/operators_unittest.cc
@@ -35,6 +35,30 @@
return list;
}
+// This parse node is for passing to tests. It returns a canned value for
+// Execute().
+class TestParseNode : public ParseNode {
+ public:
+ TestParseNode(const Value& v) : value_(v) {
+ }
+
+ Value Execute(Scope* scope, Err* err) const override {
+ return value_;
+ }
+ LocationRange GetRange() const override {
+ return LocationRange();
+ }
+ Err MakeErrorDescribing(const std::string& msg,
+ const std::string& help) const override {
+ return Err(this, msg);
+ }
+ void Print(std::ostream& out, int indent) const override {
+ }
+
+ private:
+ Value value_;
+};
+
} // namespace
TEST(Operators, SourcesAppend) {
@@ -158,6 +182,56 @@
EXPECT_TRUE(err.has_error());
}
+TEST(Operators, ListRemove) {
+ Err err;
+ TestWithScope setup;
+
+ const char foo_str[] = "foo";
+ const char bar_str[] = "bar";
+ Value test_list(nullptr, Value::LIST);
+ test_list.list_value().push_back(Value(nullptr, foo_str));
+ test_list.list_value().push_back(Value(nullptr, bar_str));
+ test_list.list_value().push_back(Value(nullptr, foo_str));
+
+ // Set up "var" with an the test list.
+ const char var_str[] = "var";
+ setup.scope()->SetValue(var_str, test_list, nullptr);
+
+ // Set up the operator.
+ BinaryOpNode node;
+ const char token_value[] = "-=";
+ Token op(Location(), Token::MINUS_EQUALS, token_value);
+ node.set_op(op);
+
+ // Do -= on the var.
+ Token identifier_token(Location(), Token::IDENTIFIER, var_str);
+ node.set_left(
+ std::unique_ptr<ParseNode>(new IdentifierNode(identifier_token)));
+
+ // Subtract a list consisting of "foo".
+ Value foo_list(nullptr, Value::LIST);
+ foo_list.list_value().push_back(Value(nullptr, foo_str));
+ std::unique_ptr<ParseNode> outer_list(new TestParseNode(foo_list));
+ node.set_right(std::move(outer_list));
+
+ Value result = ExecuteBinaryOperator(
+ setup.scope(), &node, node.left(), node.right(), &err);
+ EXPECT_FALSE(err.has_error());
+
+ // -= returns an empty value to reduce the possibility of writing confusing
+ // cases like foo = bar += 1.
+ EXPECT_EQ(Value::NONE, result.type());
+
+ // The "var" variable should have been updated. Both instances of "foo" are
+ // deleted.
+ const Value* new_value = setup.scope()->GetValue(var_str);
+ ASSERT_TRUE(new_value);
+ ASSERT_EQ(Value::LIST, new_value->type());
+ ASSERT_EQ(1u, new_value->list_value().size());
+ ASSERT_EQ(Value::STRING, new_value->list_value()[0].type());
+ EXPECT_EQ("bar", new_value->list_value()[0].string_value());
+}
+
TEST(Operators, ShortCircuitAnd) {
Err err;
TestWithScope setup;
diff --git a/tools/gn/parse_tree.cc b/tools/gn/parse_tree.cc
index dc80514..ecc438b 100644
--- a/tools/gn/parse_tree.cc
+++ b/tools/gn/parse_tree.cc
@@ -172,12 +172,6 @@
}
Value AccessorNode::ExecuteArrayAccess(Scope* scope, Err* err) const {
- Value index_value = index_->Execute(scope, err);
- if (err->has_error())
- return Value();
- if (!index_value.VerifyTypeIs(Value::INTEGER, err))
- return Value();
-
const Value* base_value = scope->GetValue(base_.value(), true);
if (!base_value) {
*err = MakeErrorDescribing("Undefined identifier.");
@@ -186,29 +180,11 @@
if (!base_value->VerifyTypeIs(Value::LIST, err))
return Value();
- int64_t index_int = index_value.int_value();
- if (index_int < 0) {
- *err = Err(index_->GetRange(), "Negative array subscript.",
- "You gave me " + base::Int64ToString(index_int) + ".");
+ size_t index = 0;
+ if (!ComputeAndValidateListIndex(scope, base_value->list_value().size(),
+ &index, err))
return Value();
- }
- size_t index_sizet = static_cast<size_t>(index_int);
- if (index_sizet >= base_value->list_value().size()) {
- *err =
- Err(index_->GetRange(), "Array subscript out of range.",
- "You gave me " + base::Int64ToString(index_int) +
- " but I was expecting something from 0 to " +
- base::Int64ToString(
- static_cast<int64_t>(base_value->list_value().size()) - 1) +
- ", inclusive.");
- return Value();
- }
-
- // Doing this assumes that there's no way in the language to do anything
- // between the time the reference is created and the time that the reference
- // is used. If there is, this will crash! Currently, this is just used for
- // array accesses where this "shouldn't" happen.
- return base_value->list_value()[index_sizet];
+ return base_value->list_value()[index];
}
Value AccessorNode::ExecuteScopeAccess(Scope* scope, Err* err) const {
@@ -222,7 +198,8 @@
const Value* result = nullptr;
// Look up the value in the scope named by "base_".
- Value* mutable_base_value = scope->GetMutableValue(base_.value(), true);
+ Value* mutable_base_value = scope->GetMutableValue(
+ base_.value(), Scope::SEARCH_NESTED, true);
if (mutable_base_value) {
// Common case: base value is mutable so we can track variable accesses
// for unused value warnings.
@@ -259,6 +236,35 @@
Location(old.file(), line_number, old.column_number(), old.byte()));
}
+bool AccessorNode::ComputeAndValidateListIndex(Scope* scope,
+ size_t max_len,
+ size_t* computed_index,
+ Err* err) const {
+ Value index_value = index_->Execute(scope, err);
+ if (err->has_error())
+ return false;
+ if (!index_value.VerifyTypeIs(Value::INTEGER, err))
+ return false;
+
+ int64_t index_int = index_value.int_value();
+ if (index_int < 0) {
+ *err = Err(index_->GetRange(), "Negative array subscript.",
+ "You gave me " + base::Int64ToString(index_int) + ".");
+ return false;
+ }
+ size_t index_sizet = static_cast<size_t>(index_int);
+ if (index_sizet >= max_len) {
+ *err = Err(index_->GetRange(), "Array subscript out of range.",
+ "You gave me " + base::Int64ToString(index_int) +
+ " but I was expecting something from 0 to " +
+ base::SizeTToString(max_len) + ", inclusive.");
+ return false;
+ }
+
+ *computed_index = index_sizet;
+ return true;
+}
+
// BinaryOpNode ---------------------------------------------------------------
BinaryOpNode::BinaryOpNode() {
@@ -293,7 +299,7 @@
// BlockNode ------------------------------------------------------------------
-BlockNode::BlockNode() {
+BlockNode::BlockNode(ResultMode result_mode) : result_mode_(result_mode) {
}
BlockNode::~BlockNode() {
@@ -303,18 +309,46 @@
return this;
}
-Value BlockNode::Execute(Scope* scope, Err* err) const {
+Value BlockNode::Execute(Scope* enclosing_scope, Err* err) const {
+ std::unique_ptr<Scope> nested_scope; // May be null.
+
+ Scope* execution_scope; // Either the enclosing_scope or nested_scope.
+ if (result_mode_ == RETURNS_SCOPE) {
+ // Create a nested scope to save the values for returning.
+ nested_scope.reset(new Scope(enclosing_scope));
+ execution_scope = nested_scope.get();
+ } else {
+ // Use the enclosing scope. Modifications will go into this also (for
+ // example, if conditions and loops).
+ execution_scope = enclosing_scope;
+ }
+
for (size_t i = 0; i < statements_.size() && !err->has_error(); i++) {
// Check for trying to execute things with no side effects in a block.
+ //
+ // A BlockNode here means that somebody has a free-floating { }.
+ // Technically this can have side effects since it could generated targets,
+ // but we don't want to allow this since it creates ambiguity when
+ // immediately following a function call that takes no block. By not
+ // allowing free-floating blocks that aren't passed anywhere or assigned to
+ // anything, this ambiguity is resolved.
const ParseNode* cur = statements_[i].get();
if (cur->AsList() || cur->AsLiteral() || cur->AsUnaryOp() ||
- cur->AsIdentifier()) {
+ cur->AsIdentifier() || cur->AsBlock()) {
*err = cur->MakeErrorDescribing(
"This statement has no effect.",
"Either delete it or do something with the result.");
return Value();
}
- cur->Execute(scope, err);
+ cur->Execute(execution_scope, err);
+ }
+
+ if (result_mode_ == RETURNS_SCOPE) {
+ // Clear the reference to the containing scope. This will be passed in
+ // a value whose lifetime will not be related to the enclosing_scope passed
+ // to this function.
+ nested_scope->DetachFromContaining();
+ return Value(this, std::move(nested_scope));
}
return Value();
}
diff --git a/tools/gn/parse_tree.h b/tools/gn/parse_tree.h
index 48313be..415041e 100644
--- a/tools/gn/parse_tree.h
+++ b/tools/gn/parse_tree.h
@@ -160,6 +160,15 @@
void SetNewLocation(int line_number);
+ // Evaluates the index for list accessor operations and range checks it
+ // against the max length of the list. If the index is OK, sets
+ // |*computed_index| and returns true. Otherwise sets the |*err| and returns
+ // false.
+ bool ComputeAndValidateListIndex(Scope* scope,
+ size_t max_len,
+ size_t* computed_index,
+ Err* err) const;
+
private:
Value ExecuteArrayAccess(Scope* scope, Err* err) const;
Value ExecuteScopeAccess(Scope* scope, Err* err) const;
@@ -212,7 +221,18 @@
class BlockNode : public ParseNode {
public:
- BlockNode();
+ // How Execute manages the scopes and results.
+ enum ResultMode {
+ // Creates a new scope for the execution of this block and returns it as
+ // a Value from Execute().
+ RETURNS_SCOPE,
+
+ // Executes in the context of the calling scope (variables set will go
+ // into the invoking scope) and Execute will return an empty Value.
+ DISCARDS_RESULT
+ };
+
+ BlockNode(ResultMode result_mode);
~BlockNode() override;
const BlockNode* AsBlock() const override;
@@ -227,6 +247,8 @@
void set_end(std::unique_ptr<EndNode> e) { end_ = std::move(e); }
const EndNode* End() const { return end_.get(); }
+ ResultMode result_mode() const { return result_mode_; }
+
const std::vector<std::unique_ptr<ParseNode>>& statements() const {
return statements_;
}
@@ -235,6 +257,8 @@
}
private:
+ const ResultMode result_mode_;
+
// Tokens corresponding to { and }, if any (may be NULL). The end is stored
// in a custom parse node so that it can have comments hung off of it.
Token begin_token_;
diff --git a/tools/gn/parse_tree_unittest.cc b/tools/gn/parse_tree_unittest.cc
index a6850a7..0609215 100644
--- a/tools/gn/parse_tree_unittest.cc
+++ b/tools/gn/parse_tree_unittest.cc
@@ -47,7 +47,7 @@
const int64_t kBValue = 42;
err = Err();
setup.scope()
- ->GetMutableValue("a", false)
+ ->GetMutableValue("a", Scope::SEARCH_NESTED, false)
->scope_value()
->SetValue("b", Value(nullptr, kBValue), nullptr);
result = accessor.Execute(setup.scope(), &err);
diff --git a/tools/gn/parser.cc b/tools/gn/parser.cc
index 7e5886f..57a398e 100644
--- a/tools/gn/parser.cc
+++ b/tools/gn/parser.cc
@@ -13,7 +13,7 @@
#include "tools/gn/token.h"
const char kGrammar_Help[] =
- "GN build language grammar\n"
+ "Language and grammar for GN build files\n"
"\n"
"Tokens\n"
"\n"
@@ -79,6 +79,13 @@
" To insert an arbitrary byte value, use $0xFF. For example, to\n"
" insert a newline character: \"Line one$0x0ALine two\".\n"
"\n"
+ " An expansion will evaluate the variable following the '$' and insert\n"
+ " a stringified version of it into the result. For example, to concat\n"
+ " two path components with a slash separating them:\n"
+ " \"$var_one/$var_two\"\n"
+ " Use the \"${var_one}\" format to be explicitly deliniate the variable\n"
+ " for otherwise-ambiguous cases.\n"
+ "\n"
"Punctuation\n"
"\n"
" The following character sequences represent punctuation:\n"
@@ -95,19 +102,20 @@
" File = StatementList .\n"
"\n"
" Statement = Assignment | Call | Condition .\n"
- " Assignment = identifier AssignOp Expr .\n"
+ " LValue = identifier | ArrayAccess | ScopeAccess .\n"
+ " Assignment = LValue AssignOp Expr .\n"
" Call = identifier \"(\" [ ExprList ] \")\" [ Block ] .\n"
" Condition = \"if\" \"(\" Expr \")\" Block\n"
" [ \"else\" ( Condition | Block ) ] .\n"
" Block = \"{\" StatementList \"}\" .\n"
" StatementList = { Statement } .\n"
"\n"
- " ArrayAccess = identifier \"[\" { identifier | integer } \"]\" .\n"
+ " ArrayAccess = identifier \"[\" Expr \"]\" .\n"
" ScopeAccess = identifier \".\" identifier .\n"
" Expr = UnaryExpr | Expr BinaryOp Expr .\n"
" UnaryExpr = PrimaryExpr | UnaryOp UnaryExpr .\n"
" PrimaryExpr = identifier | integer | string | Call\n"
- " | ArrayAccess | ScopeAccess\n"
+ " | ArrayAccess | ScopeAccess | Block\n"
" | \"(\" Expr \")\"\n"
" | \"[\" [ ExprList [ \",\" ] ] \"]\" .\n"
" ExprList = Expr { \",\" Expr } .\n"
@@ -120,7 +128,95 @@
" | \"&&\"\n"
" | \"||\" . // lowest priority\n"
"\n"
- " All binary operators are left-associative.\n";
+ " All binary operators are left-associative.\n"
+ "\n"
+ "Types\n"
+ "\n"
+ " The GN language is dynamically typed. The following types are used:\n"
+ "\n"
+ " - Boolean: Uses the keywords \"true\" and \"false\". There is no\n"
+ " implicit conversion between booleans and integers.\n"
+ "\n"
+ " - Integers: All numbers in GN are signed 64-bit integers.\n"
+ "\n"
+ " - Strings: Strings are 8-bit with no enforced encoding. When a string\n"
+ " is used to interact with other systems with particular encodings\n"
+ " (like the Windows and Mac filesystems) it is assumed to be UTF-8.\n"
+ " See \"String literals\" above for more.\n"
+ "\n"
+ " - Lists: Lists are arbitrary-length ordered lists of values. See\n"
+ " \"Lists\" below for more.\n"
+ "\n"
+ " - Scopes: Scopes are like dictionaries that use variable names for\n"
+ " keys. See \"Scopes\" below for more.\n"
+ "\n"
+ "Lists\n"
+ "\n"
+ " Lists are created with [] and using commas to separate items:\n"
+ "\n"
+ " mylist = [ 0, 1, 2, \"some string\" ]\n"
+ "\n"
+ " A comma after the last item is optional. Lists are dereferenced using\n"
+ " 0-based indexing:\n"
+ "\n"
+ " mylist[0] += 1\n"
+ " var = mylist[2]\n"
+ "\n"
+ " Lists can be concatenated using the '+' and '+=' operators. Bare\n"
+ " values can not be concatenated with lists, to add a single item,\n"
+ " it must be put into a list of length one.\n"
+ "\n"
+ " Items can be removed from lists using the '-' and '-=' operators.\n"
+ " This will remove all occurrences of every item in the right-hand list\n"
+ " from the left-hand list. It is an error to remove an item not in the\n"
+ " list. This is to prevent common typos and to detect dead code that\n"
+ " is removing things that no longer apply.\n"
+ "\n"
+ " It is an error to use '=' to replace a nonempty list with another\n"
+ " nonempty list. This is to prevent accidentally overwriting data\n"
+ " when in most cases '+=' was intended. To overwrite a list on purpose,\n"
+ " first assign it to the empty list:\n"
+ "\n"
+ " mylist = []\n"
+ " mylist = otherlist\n"
+ "\n"
+ " When assigning to a list named 'sources' using '=' or '+=', list\n"
+ " items may be automatically filtered out.\n"
+ " See \"gn help set_sources_assignment_filter\" for more.\n"
+ "\n"
+ "Scopes\n"
+ "\n"
+ " All execution happens in the context of a scope which holds the\n"
+ " current state (like variables). With the exception of loops and\n"
+ " conditions, '{' introduces a new scope that has a parent reference to\n"
+ " the old scope.\n"
+ "\n"
+ " Variable reads recursively search all nested scopes until the\n"
+ " variable is found or there are no more scopes. Variable writes always\n"
+ " go into the current scope. This means that after the closing '}'\n"
+ " (again excepting loops and conditions), all local variables will be\n"
+ " restored to the previous values. This also means that \"foo = foo\"\n"
+ " can do useful work by copying a variable into the current scope that\n"
+ " was defined in a containing scope.\n"
+ "\n"
+ " Scopes can also be assigned to variables. Such scopes can be created\n"
+ " by functions like exec_script, when invoking a template (the template\n"
+ " code refers to the variables set by the invoking code by the\n"
+ " implicitly-created \"invoker\" scope), or explicitly like:\n"
+ "\n"
+ " empty_scope = {}\n"
+ " myvalues = {\n"
+ " foo = 21\n"
+ " bar = \"something\"\n"
+ " }\n"
+ "\n"
+ " Inside such a scope definition can be any GN code including\n"
+ " conditionals and function calls. After the close of the scope, it will\n"
+ " contain all variables explicitly set by the code contained inside it.\n"
+ " After this, the values can be read, modified, or added to:\n"
+ "\n"
+ " myvalues.foo += 2\n"
+ " empty_scope.new_thing = [ 1, 2, 3 ]\n";
enum Precedence {
PRECEDENCE_ASSIGNMENT = 1, // Lowest precedence.
@@ -172,7 +268,7 @@
{nullptr, nullptr, -1}, // RIGHT_PAREN
{&Parser::List, &Parser::Subscript, PRECEDENCE_CALL}, // LEFT_BRACKET
{nullptr, nullptr, -1}, // RIGHT_BRACKET
- {nullptr, nullptr, -1}, // LEFT_BRACE
+ {&Parser::Block, nullptr, -1}, // LEFT_BRACE
{nullptr, nullptr, -1}, // RIGHT_BRACE
{nullptr, nullptr, -1}, // IF
{nullptr, nullptr, -1}, // ELSE
@@ -356,6 +452,12 @@
return left;
}
+std::unique_ptr<ParseNode> Parser::Block(Token token) {
+ // This entrypoing into ParseBlock means its part of an expression and we
+ // always want the result.
+ return ParseBlock(token, BlockNode::RETURNS_SCOPE);
+}
+
std::unique_ptr<ParseNode> Parser::Literal(Token token) {
return base::WrapUnique(new LiteralNode(token));
}
@@ -441,7 +543,7 @@
}
// Optionally with a scope.
if (LookAhead(Token::LEFT_BRACE)) {
- block = ParseBlock();
+ block = ParseBlock(Consume(), BlockNode::DISCARDS_RESULT);
if (has_error())
return std::unique_ptr<ParseNode>();
}
@@ -461,8 +563,10 @@
std::unique_ptr<ParseNode> Parser::Assignment(std::unique_ptr<ParseNode> left,
Token token) {
- if (left->AsIdentifier() == nullptr) {
- *err_ = Err(left.get(), "Left-hand side of assignment must be identifier.");
+ if (left->AsIdentifier() == nullptr && left->AsAccessor() == nullptr) {
+ *err_ = Err(left.get(),
+ "The left-hand side of an assignment must be an identifier, "
+ "scope access, or array access.");
return std::unique_ptr<ParseNode>();
}
std::unique_ptr<ParseNode> value = ParseExpression(PRECEDENCE_ASSIGNMENT);
@@ -567,7 +671,7 @@
}
std::unique_ptr<ParseNode> Parser::ParseFile() {
- std::unique_ptr<BlockNode> file(new BlockNode);
+ std::unique_ptr<BlockNode> file(new BlockNode(BlockNode::DISCARDS_RESULT));
for (;;) {
if (at_end())
break;
@@ -611,13 +715,13 @@
}
}
-std::unique_ptr<BlockNode> Parser::ParseBlock() {
- Token begin_token =
- Consume(Token::LEFT_BRACE, "Expected '{' to start a block.");
+std::unique_ptr<BlockNode> Parser::ParseBlock(
+ Token begin_brace,
+ BlockNode::ResultMode result_mode) {
if (has_error())
return std::unique_ptr<BlockNode>();
- std::unique_ptr<BlockNode> block(new BlockNode);
- block->set_begin_token(begin_token);
+ std::unique_ptr<BlockNode> block(new BlockNode(result_mode));
+ block->set_begin_token(begin_brace);
for (;;) {
if (LookAhead(Token::RIGHT_BRACE)) {
@@ -641,10 +745,13 @@
if (IsAssignment(condition->condition()))
*err_ = Err(condition->condition(), "Assignment not allowed in 'if'.");
Consume(Token::RIGHT_PAREN, "Expected ')' after condition of 'if'.");
- condition->set_if_true(ParseBlock());
+ condition->set_if_true(ParseBlock(
+ Consume(Token::LEFT_BRACE, "Expected '{' to start 'if' block."),
+ BlockNode::DISCARDS_RESULT));
if (Match(Token::ELSE)) {
if (LookAhead(Token::LEFT_BRACE)) {
- condition->set_if_false(ParseBlock());
+ condition->set_if_false(ParseBlock(Consume(),
+ BlockNode::DISCARDS_RESULT));
} else if (LookAhead(Token::IF)) {
condition->set_if_false(ParseStatement());
} else {
diff --git a/tools/gn/parser.h b/tools/gn/parser.h
index de828a4..29580da 100644
--- a/tools/gn/parser.h
+++ b/tools/gn/parser.h
@@ -58,6 +58,7 @@
std::unique_ptr<ParseNode> ParseExpression(int precedence);
// |PrefixFunc|s used in parsing expressions.
+ std::unique_ptr<ParseNode> Block(Token token);
std::unique_ptr<ParseNode> Literal(Token token);
std::unique_ptr<ParseNode> Name(Token token);
std::unique_ptr<ParseNode> Group(Token token);
@@ -85,7 +86,10 @@
std::unique_ptr<ParseNode> ParseFile();
std::unique_ptr<ParseNode> ParseStatement();
- std::unique_ptr<BlockNode> ParseBlock();
+ // Expects to be passed the token corresponding to the '{' and that the
+ // current token is the one following the '{'.
+ std::unique_ptr<BlockNode> ParseBlock(Token being_brace,
+ BlockNode::ResultMode result_mode);
std::unique_ptr<ParseNode> ParseCondition();
// Generates a pre- and post-order traversal of the tree.
diff --git a/tools/gn/parser_unittest.cc b/tools/gn/parser_unittest.cc
index 3970373..6caeeb4 100644
--- a/tools/gn/parser_unittest.cc
+++ b/tools/gn/parser_unittest.cc
@@ -246,8 +246,14 @@
" b\n"
" IDENTIFIER(c)\n"
" LITERAL(2)\n");
+ DoParserPrintTest("a.b = 5",
+ "BLOCK\n"
+ " BINARY(=)\n"
+ " ACCESSOR\n"
+ " a\n"
+ " IDENTIFIER(b)\n"
+ " LITERAL(5)\n");
DoParserErrorTest("a = b.c.d", 1, 6); // Can't nest accessors (currently).
- DoParserErrorTest("a.b = 5", 1, 1); // Can't assign to accessors (currently).
// Error at the bad dot in the RHS, not the + operator (crbug.com/472038).
DoParserErrorTest("foo(a + b.c.d)", 1, 10);
@@ -701,11 +707,38 @@
// a function with an associated block, or a standalone function with a
// freestanding block.
TEST(Parser, StandaloneBlock) {
+ // The error is reported at the end of the block when nothing is done
+ // with it. If we had said "a = { ..." then it would have been OK.
DoParserErrorTest(
"if (true) {\n"
"}\n"
"{\n"
" assert(false)\n"
"}\n",
- 3, 1);
+ 5, 1);
+}
+
+TEST(Parser, BlockValues) {
+ const char* input =
+ "print({a = 1 b = 2}, 3)\n"
+ "a = { b = \"asd\" }";
+ const char* expected =
+ "BLOCK\n"
+ " FUNCTION(print)\n"
+ " LIST\n"
+ " BLOCK\n"
+ " BINARY(=)\n"
+ " IDENTIFIER(a)\n"
+ " LITERAL(1)\n"
+ " BINARY(=)\n"
+ " IDENTIFIER(b)\n"
+ " LITERAL(2)\n"
+ " LITERAL(3)\n"
+ " BINARY(=)\n"
+ " IDENTIFIER(a)\n"
+ " BLOCK\n"
+ " BINARY(=)\n"
+ " IDENTIFIER(b)\n"
+ " LITERAL(\"asd\")\n";
+ DoParserPrintTest(input, expected);
}
diff --git a/tools/gn/scope.cc b/tools/gn/scope.cc
index 8e9230d..7e7c201 100644
--- a/tools/gn/scope.cc
+++ b/tools/gn/scope.cc
@@ -66,6 +66,11 @@
Scope::~Scope() {
}
+void Scope::DetachFromContaining() {
+ const_containing_ = nullptr;
+ mutable_containing_ = nullptr;
+}
+
const Value* Scope::GetValue(const base::StringPiece& ident,
bool counts_as_used) {
// First check for programmatically-provided values.
@@ -91,6 +96,7 @@
}
Value* Scope::GetMutableValue(const base::StringPiece& ident,
+ SearchNested search_mode,
bool counts_as_used) {
// Don't do programmatic values, which are not mutable.
RecordMap::iterator found = values_.find(ident);
@@ -100,25 +106,10 @@
return &found->second.value;
}
- // Search in the parent mutable scope, but not const one.
- if (mutable_containing_)
- return mutable_containing_->GetMutableValue(ident, counts_as_used);
- return nullptr;
-}
-
-Value* Scope::GetValueForcedToCurrentScope(const base::StringPiece& ident,
- const ParseNode* set_node) {
- RecordMap::iterator found = values_.find(ident);
- if (found != values_.end())
- return &found->second.value; // Already have in the current scope.
-
- // Search in the parent scope.
- if (containing()) {
- const Value* in_containing = containing()->GetValue(ident);
- if (in_containing) {
- // Promote to current scope.
- return SetValue(ident, *in_containing, set_node);
- }
+ // Search in the parent mutable scope if requested, but not const one.
+ if (search_mode == SEARCH_NESTED && mutable_containing_) {
+ return mutable_containing_->GetMutableValue(
+ ident, Scope::SEARCH_NESTED, counts_as_used);
}
return nullptr;
}
@@ -144,10 +135,10 @@
}
Value* Scope::SetValue(const base::StringPiece& ident,
- const Value& v,
+ Value v,
const ParseNode* set_node) {
Record& r = values_[ident]; // Clears any existing value.
- r.value = v;
+ r.value = std::move(v);
r.value.set_origin(set_node);
return &r.value;
}
diff --git a/tools/gn/scope.h b/tools/gn/scope.h
index 685f840..3d36712 100644
--- a/tools/gn/scope.h
+++ b/tools/gn/scope.h
@@ -45,6 +45,13 @@
// Holds an owning list of Items.
typedef ScopedVector<Item> ItemVector;
+ // A flag to indicate whether a function should recurse into nested scopes,
+ // or only operate on the current scope.
+ enum SearchNested {
+ SEARCH_NESTED,
+ SEARCH_CURRENT
+ };
+
// Allows code to provide values for built-in variables. This class will
// automatically register itself on construction and deregister itself on
// destruction.
@@ -114,6 +121,10 @@
return mutable_containing_ ? mutable_containing_ : const_containing_;
}
+ // Clears any references to containing scopes. This scope will now be
+ // self-sufficient.
+ void DetachFromContaining();
+
// Returns NULL if there's no such value.
//
// counts_as_used should be set if the variable is being read in a way that
@@ -122,6 +133,16 @@
bool counts_as_used);
const Value* GetValue(const base::StringPiece& ident) const;
+ // If the value exists in the current scope, destrictively moves it into the
+ // return value. If it exists in a containing scope, copies it.
+ //
+ // This is for implementing modify-write operations where we want to read
+ // the existing value and plan to immediately overwrite it. If the value is
+ // in a containing scope, we never want to touch it (all writes go to the
+ // current scope), but if it's in the current scope, avoid the copy since it
+ // will be overwritten anyway.
+ //Value DestructiveMoveOut(const base::StringPiece& ident);
+
// Returns the requested value as a mutable one if possible. If the value
// is not found in a mutable scope, then returns null. Note that the value
// could still exist in a const scope, so GetValue() could still return
@@ -144,17 +165,9 @@
// }
// The 6 should get set on the nested scope rather than modify the value
// in the outer one.
- Value* GetMutableValue(const base::StringPiece& ident, bool counts_as_used);
-
- // Same as GetValue, but if the value exists in a parent scope, we'll copy
- // it to the current scope. If the return value is non-null, the value is
- // guaranteed to be set in the current scope. Generatlly this will be used
- // if the calling code is planning on modifying the value in-place.
- //
- // Since this is used when doing read-modifies, we never count this access
- // as reading the variable, since we assume it will be written to.
- Value* GetValueForcedToCurrentScope(const base::StringPiece& ident,
- const ParseNode* set_node);
+ Value* GetMutableValue(const base::StringPiece& ident,
+ SearchNested search_mode,
+ bool counts_as_used);
// Returns the StringPiece used to identify the value. This string piece
// will have the same contents as "ident" passed in, but may point to a
@@ -168,7 +181,7 @@
// errors later. Returns a pointer to the value in the current scope (a copy
// is made for storage).
Value* SetValue(const base::StringPiece& ident,
- const Value& v,
+ Value v,
const ParseNode* set_node);
// Removes the value with the given identifier if it exists on the current
diff --git a/tools/gn/scope_unittest.cc b/tools/gn/scope_unittest.cc
index de2005a..a90d725 100644
--- a/tools/gn/scope_unittest.cc
+++ b/tools/gn/scope_unittest.cc
@@ -262,10 +262,12 @@
// Check getting root scope values.
EXPECT_TRUE(mutable_scope2.GetValue(kOnConst, true));
- EXPECT_FALSE(mutable_scope2.GetMutableValue(kOnConst, true));
+ EXPECT_FALSE(mutable_scope2.GetMutableValue(
+ kOnConst, Scope::SEARCH_NESTED, true));
// Test reading a value from scope 1.
- Value* mutable1_result = mutable_scope2.GetMutableValue(kOnMutable1, false);
+ Value* mutable1_result = mutable_scope2.GetMutableValue(
+ kOnMutable1, Scope::SEARCH_NESTED, false);
ASSERT_TRUE(mutable1_result);
EXPECT_TRUE(*mutable1_result == value);
@@ -273,13 +275,15 @@
// used in the previous step).
Err err;
EXPECT_FALSE(mutable_scope1.CheckForUnusedVars(&err));
- mutable1_result = mutable_scope2.GetMutableValue(kOnMutable1, true);
+ mutable1_result = mutable_scope2.GetMutableValue(
+ kOnMutable1, Scope::SEARCH_NESTED, true);
EXPECT_TRUE(mutable1_result);
err = Err();
EXPECT_TRUE(mutable_scope1.CheckForUnusedVars(&err));
// Test reading a value from scope 2.
- Value* mutable2_result = mutable_scope2.GetMutableValue(kOnMutable2, true);
+ Value* mutable2_result = mutable_scope2.GetMutableValue(
+ kOnMutable2, Scope::SEARCH_NESTED, true);
ASSERT_TRUE(mutable2_result);
EXPECT_TRUE(*mutable2_result == value);
}
diff --git a/tools/gn/template.cc b/tools/gn/template.cc
index e865264..66d6745 100644
--- a/tools/gn/template.cc
+++ b/tools/gn/template.cc
@@ -80,8 +80,8 @@
// be avoided.
template_scope.SetValue(variables::kInvoker,
Value(nullptr, std::unique_ptr<Scope>()), invocation);
- Value* invoker_value =
- template_scope.GetMutableValue(variables::kInvoker, false);
+ Value* invoker_value = template_scope.GetMutableValue(
+ variables::kInvoker, Scope::SEARCH_NESTED, false);
invoker_value->SetScopeValue(std::move(invocation_scope));
template_scope.set_source_dir(scope->GetSourceDir());
@@ -108,7 +108,8 @@
// to overwrite the value of "invoker" and free the Scope owned by the
// value. So we need to look it up again and don't do anything if it doesn't
// exist.
- invoker_value = template_scope.GetMutableValue(variables::kInvoker, false);
+ invoker_value = template_scope.GetMutableValue(
+ variables::kInvoker, Scope::SEARCH_NESTED, false);
if (invoker_value && invoker_value->type() == Value::SCOPE) {
if (!invoker_value->scope_value()->CheckForUnusedVars(err))
return Value();
diff --git a/tools/gn/value.cc b/tools/gn/value.cc
index 9a52fba..b5e21f0 100644
--- a/tools/gn/value.cc
+++ b/tools/gn/value.cc
@@ -73,6 +73,8 @@
scope_value_ = other.scope_value_->MakeClosure();
}
+Value::Value(Value&& other) = default;
+
Value::~Value() {
}
diff --git a/tools/gn/value.h b/tools/gn/value.h
index a8a83fb..0428818 100644
--- a/tools/gn/value.h
+++ b/tools/gn/value.h
@@ -43,9 +43,11 @@
Value(const ParseNode* origin, std::unique_ptr<Scope> scope);
Value(const Value& other);
+ Value(Value&& other);
~Value();
Value& operator=(const Value& other);
+ Value& operator=(Value&& other) = default;
Type type() const { return type_; }
diff --git a/tools/gn/visibility_unittest.cc b/tools/gn/visibility_unittest.cc
index e120f74..7d59540 100644
--- a/tools/gn/visibility_unittest.cc
+++ b/tools/gn/visibility_unittest.cc
@@ -5,6 +5,7 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "tools/gn/err.h"
#include "tools/gn/label.h"
+#include "tools/gn/scope.h"
#include "tools/gn/value.h"
#include "tools/gn/visibility.h"
