From 6b4ee78fae44556e1204d9093d01b06ac5ef4af1 Mon Sep 17 00:00:00 2001 From: "brettw@chromium.org" Date: Tue, 20 Aug 2013 17:13:24 +0000 Subject: [PATCH] Fix template invocations in GN. This passes the right tokens to block begin/end for setup, and changed the Tokens to be copies instead of pointers. I separate out the set_defaults function into its own file and document it. I fixed a bug with template invocation where I did data() on a StringPiece to pass as a null terminated C-string which doesn't work at all. R=scottmg@chromium.org Review URL: https://codereview.chromium.org/22859028 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@218509 0039d316-1c4b-4281-b951-d872f2087c98 --- tools/gn/BUILD.gn | 1 + tools/gn/function_set_defaults.cc | 89 +++++++++++++++++++++++++++++++ tools/gn/functions.cc | 59 ++++---------------- tools/gn/functions.h | 6 ++- tools/gn/gn.gyp | 1 + tools/gn/parse_tree.cc | 23 ++++---- tools/gn/parse_tree.h | 8 +-- tools/gn/parser.cc | 14 +++-- tools/gn/token.h | 3 +- 9 files changed, 131 insertions(+), 73 deletions(-) create mode 100644 tools/gn/function_set_defaults.cc diff --git a/tools/gn/BUILD.gn b/tools/gn/BUILD.gn index 8b5e67052a8145..0d5f6cc44b1a4e 100644 --- a/tools/gn/BUILD.gn +++ b/tools/gn/BUILD.gn @@ -38,6 +38,7 @@ static_library("gn_lib") { "function_process_file_template.cc", "function_read_file.cc", "function_set_default_toolchain.cc", + "function_set_defaults.cc", "function_template.cc", "function_toolchain.cc", "function_write_file.cc", diff --git a/tools/gn/function_set_defaults.cc b/tools/gn/function_set_defaults.cc new file mode 100644 index 00000000000000..45c6fc59edbdfb --- /dev/null +++ b/tools/gn/function_set_defaults.cc @@ -0,0 +1,89 @@ +// Copyright (c) 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "tools/gn/err.h" +#include "tools/gn/functions.h" +#include "tools/gn/parse_tree.h" +#include "tools/gn/scope.h" + +namespace functions { + +const char kSetDefaults[] = "set_defaults"; +const char kSetDefaults_Help[] = + "set_defaults: Set default values for a target type.\n" + "\n" + " set_defaults() { }\n" + "\n" + " Sets the default values for a given target type. Whenever\n" + " target_type_name is seen in the future, the values specified in\n" + " set_default's block will be copied into the current scope.\n" + "\n" + " When the target type is used, the variable copying is very strict.\n" + " If a variable with that name is already in scope, the build will fail\n" + " with an error.\n" + "\n" + " set_defaults can be used for built-in target types (\"executable\",\n" + " \"shared_library\", etc.) and custom ones defined via the \"template\"\n" + " command.\n" + "\n" + "Example:\n" + " set_defaults(\"static_library\") {\n" + " configs = [ \"//tools/mything:settings\" ]\n" + " }\n" + "\n" + " static_library(\"mylib\")\n" + " # The configs will be auto-populated as above. You can remove it if\n" + " # you don't want the default for a particular default:\n" + " configs -= \"//tools/mything:setgings\"\n" + " }\n"; + +Value RunSetDefaults(Scope* scope, + const FunctionCallNode* function, + const std::vector& args, + BlockNode* block, + Err* err) { + if (!EnsureSingleStringArg(function, args, err)) + return Value(); + const std::string& target_type(args[0].string_value()); + + // Ensure there aren't defaults already set. + // + // It might be nice to allow multiple calls set mutate the defaults. The + // main case for this is where some local portions of the code want + // additional defaults they specify in an imported file. + // + // Currently, we don't allow imports to clobber anything, so this wouldn't + // work. Additionally, allowing this would be undesirable since we don't + // want multiple imports to each try to set defaults, since it might look + // like the defaults are modified by each one in sequence, while in fact + // imports would always clobber previous values and it would be confusing. + // + // If we wanted this, the solution would be to allow imports to overwrite + // target defaults set up by the default build config only. That way there + // are no ordering issues, but this would be more work. + if (scope->GetTargetDefaults(target_type)) { + *err = Err(function->function(), + "This target type defaults were already set."); + return Value(); + } + + if (!block) { + FillNeedsBlockError(function, err); + return Value(); + } + + // Run the block for the rule invocation. + Scope block_scope(scope); + block->ExecuteBlockInScope(&block_scope, err); + if (err->has_error()) + return Value(); + + // Now copy the values set on the scope we made into the free-floating one + // (with no containing scope) used to hold the target defaults. + Scope* dest = scope->MakeTargetDefaults(target_type); + block_scope.NonRecursiveMergeTo(dest, function, "", err); + return Value(); +} + +} // namespace functions diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc index 3eb04428151cab..2da5ef5918a686 100644 --- a/tools/gn/functions.cc +++ b/tools/gn/functions.cc @@ -22,12 +22,6 @@ namespace { -void FillNeedsBlockError(const FunctionCallNode* function, Err* err) { - *err = Err(function->function(), "This function call requires a block.", - "The block's \"{\" must be on the same line as the function " - "call's \")\"."); -} - // This is called when a template is invoked. When we see a template // declaration, that funciton is RunTemplate. Value RunTemplateInvocation(Scope* scope, @@ -38,9 +32,10 @@ Value RunTemplateInvocation(Scope* scope, Err* err) { if (!EnsureNotProcessingImport(invocation, scope, err)) return Value(); + Scope block_scope(scope); if (!FillTargetBlockScope(scope, invocation, - invocation->function().value().data(), + invocation->function().value().as_string(), block, args, &block_scope, err)) return Value(); @@ -90,7 +85,7 @@ bool EnsureNotProcessingBuildConfig(const ParseNode* node, bool FillTargetBlockScope(const Scope* scope, const FunctionCallNode* function, - const char* target_type, + const std::string& target_type, const BlockNode* block, const std::vector& args, Scope* block_scope, @@ -122,6 +117,12 @@ bool FillTargetBlockScope(const Scope* scope, return true; } +void FillNeedsBlockError(const FunctionCallNode* function, Err* err) { + *err = Err(function->function(), "This function call requires a block.", + "The block's \"{\" must be on the same line as the function " + "call's \")\"."); +} + bool EnsureSingleStringArg(const FunctionCallNode* function, const std::vector& args, Err* err) { @@ -325,48 +326,6 @@ Value RunImport(Scope* scope, return Value(); } -// set_defaults ---------------------------------------------------------------- - -const char kSetDefaults[] = "set_defaults"; -const char kSetDefaults_Help[] = - "TODO(brettw) write this."; - -Value RunSetDefaults(Scope* scope, - const FunctionCallNode* function, - const std::vector& args, - BlockNode* block, - Err* err) { - if (!EnsureSingleStringArg(function, args, err)) - return Value(); - const std::string& target_type(args[0].string_value()); - - // Ensure there aren't defaults already set. - if (scope->GetTargetDefaults(target_type)) { - *err = Err(function->function(), - "This target type defaults were already set."); - return Value(); - } - - // Execute the block in a new scope that has a parent of the containing - // scope. - Scope block_scope(scope); - if (!FillTargetBlockScope(scope, function, - function->function().value().data(), - block, args, &block_scope, err)) - return Value(); - - // Run the block for the rule invocation. - block->ExecuteBlockInScope(&block_scope, err); - if (err->has_error()) - return Value(); - - // Now copy the values set on the scope we made into the free-floating one - // (with no containing scope) used to hold the target defaults. - Scope* dest = scope->MakeTargetDefaults(target_type); - block_scope.NonRecursiveMergeTo(dest, function, "", err); - return Value(); -} - // set_sources_assignment_filter ----------------------------------------------- const char kSetSourcesAssignmentFilter[] = "set_sources_assignment_filter"; diff --git a/tools/gn/functions.h b/tools/gn/functions.h index 9079f5bddfd90e..b55ff3a5695a64 100644 --- a/tools/gn/functions.h +++ b/tools/gn/functions.h @@ -283,12 +283,16 @@ bool EnsureNotProcessingBuildConfig(const ParseNode* node, // On success, returns true. On failure, sets the error and returns false. bool FillTargetBlockScope(const Scope* scope, const FunctionCallNode* function, - const char* target_type, + const std::string& target_type, const BlockNode* block, const std::vector& args, Scope* block_scope, Err* err); +// Sets the given error to a message explaining that the function call requires +// a block. +void FillNeedsBlockError(const FunctionCallNode* function, Err* err); + // Validates that the given function call has one string argument. This is // the most common function signature, so it saves space to have this helper. // Returns false and sets the error on failure. diff --git a/tools/gn/gn.gyp b/tools/gn/gn.gyp index 18f592bca51558..f86a520e8be172 100644 --- a/tools/gn/gn.gyp +++ b/tools/gn/gn.gyp @@ -48,6 +48,7 @@ 'function_process_file_template.cc', 'function_read_file.cc', 'function_set_default_toolchain.cc', + 'function_set_defaults.cc', 'function_template.cc', 'function_toolchain.cc', 'function_write_file.cc', diff --git a/tools/gn/parse_tree.cc b/tools/gn/parse_tree.cc index 8e1adde47aa045..613c79724cf49c 100644 --- a/tools/gn/parse_tree.cc +++ b/tools/gn/parse_tree.cc @@ -139,10 +139,7 @@ void BinaryOpNode::Print(std::ostream& out, int indent) const { // BlockNode ------------------------------------------------------------------ -BlockNode::BlockNode(bool has_scope) - : has_scope_(has_scope), - begin_token_(NULL), - end_token_(NULL) { +BlockNode::BlockNode(bool has_scope) : has_scope_(has_scope) { } BlockNode::~BlockNode() { @@ -168,18 +165,19 @@ Value BlockNode::Execute(Scope* containing_scope, Err* err) const { } LocationRange BlockNode::GetRange() const { - if (begin_token_ && end_token_) { - return begin_token_->range().Union(end_token_->range()); + if (begin_token_.type() != Token::INVALID && + end_token_.type() != Token::INVALID) { + return begin_token_.range().Union(end_token_.range()); + } else if (!statements_.empty()) { + return statements_[0]->GetRange().Union( + statements_[statements_.size() - 1]->GetRange()); } - return LocationRange(); // TODO(brettw) indicate the entire file somehow. + return LocationRange(); } Err BlockNode::MakeErrorDescribing(const std::string& msg, const std::string& help) const { - if (begin_token_) - return Err(*begin_token_, msg, help); - // TODO(brettw) this should have the beginning of the file in it or something. - return Err(Location(NULL, 1, 1), msg, help); + return Err(GetRange(), msg, help); } void BlockNode::Print(std::ostream& out, int indent) const { @@ -413,9 +411,6 @@ Value LiteralNode::Execute(Scope* scope, Err* err) const { return Value(this, result_int); } case Token::STRING: { - // TODO(brettw) Unescaping probably needs to be moved & improved. - // The input value includes the quotes around the string, strip those - // off and unescape. Value v(this, Value::STRING); ExpandStringLiteral(scope, value_, &v, err); return v; diff --git a/tools/gn/parse_tree.h b/tools/gn/parse_tree.h index 87c27728613059..69f05e1c322077 100644 --- a/tools/gn/parse_tree.h +++ b/tools/gn/parse_tree.h @@ -148,8 +148,8 @@ class BlockNode : public ParseNode { const std::string& help = std::string()) const OVERRIDE; virtual void Print(std::ostream& out, int indent) const OVERRIDE; - void set_begin_token(const Token* t) { begin_token_ = t; } - void set_end_token(const Token* t) { end_token_ = t; } + void set_begin_token(const Token& t) { begin_token_ = t; } + void set_end_token(const Token& t) { end_token_ = t; } const std::vector& statements() const { return statements_; } void append_statement(scoped_ptr s) { @@ -163,8 +163,8 @@ class BlockNode : public ParseNode { bool has_scope_; // Tokens corresponding to { and }, if any (may be NULL). - const Token* begin_token_; - const Token* end_token_; + Token begin_token_; + Token end_token_; // Owning pointers, use unique_ptr when we can use C++11. std::vector statements_; diff --git a/tools/gn/parser.cc b/tools/gn/parser.cc index 47e74a3fdd6cd4..567d84638e4d67 100644 --- a/tools/gn/parser.cc +++ b/tools/gn/parser.cc @@ -397,14 +397,22 @@ scoped_ptr Parser::ParseStatement() { } scoped_ptr Parser::ParseBlock() { - Consume(Token::LEFT_BRACE, "Expected '{' to start a block."); + Token begin_token = + Consume(Token::LEFT_BRACE, "Expected '{' to start a block."); + if (has_error()) + return scoped_ptr(); scoped_ptr block(new BlockNode(true)); + block->set_begin_token(begin_token); + for (;;) { - if (Match(Token::RIGHT_BRACE)) + if (LookAhead(Token::RIGHT_BRACE)) { + block->set_end_token(Consume()); break; + } + scoped_ptr statement = ParseStatement(); if (!statement) - break; + return scoped_ptr(); block->append_statement(statement.Pass()); } return block.Pass(); diff --git a/tools/gn/token.h b/tools/gn/token.h index 3a572cd4c3a608..4af7e624ec25fd 100644 --- a/tools/gn/token.h +++ b/tools/gn/token.h @@ -58,7 +58,8 @@ class Token { LocationRange range() const { return LocationRange(location_, Location(location_.file(), location_.line_number(), - location_.char_offset() + value_.size())); + location_.char_offset() + + static_cast(value_.size()))); } // Helper functions for comparing this token to something.