Support for excluding variable from forwarding via forward_variables_form.

Support for excluding variable from forwarding via forward_variables_form.

Some templates wants to forward all their parameters to the underlying
target except a select few they use. Previously they had to list all
the variables the underlying target supported which was error prone and
brittle if the underlying target is changed.

With this change they can just use the following:

  forward_variables_from(invoker, "*", ["my_extra_variable"])
  # my_extra_variable is not forwarded, all other variables are.

BUG=580293
Committed: https://crrev.com/bf5ff79020d15e683a12b45be08200ab7d857550
Cr-Commit-Position: refs/heads/master@{#371492}

[sdefresne, 2016-01-25 12:08:20]

sdefresne@chromium.org changed reviewers:
+ brettw@chromium.org

[sdefresne, 2016-01-25 12:08:20]

Please take a look.

[tfarina, 2016-01-25 12:15:26]

https://codereview.chromium.org/1632573002/diff/1/tools/gn/function_forward_v...
File tools/gn/function_forward_variables_from.cc (right):

https://codereview.chromium.org/1632573002/diff/1/tools/gn/function_forward_v...
tools/gn/function_forward_variables_from.cc:158: if (args_vector.size() != 2 &&
args_vector.size() != 3) {
just args_vector.size() != 3 right?

https://codereview.chromium.org/1632573002/diff/1/tools/gn/function_forward_v...
tools/gn/function_forward_variables_from.cc:160: "Expecting exactly two.");
update this error message?

[sdefresne, 2016-01-25 15:28:30]

https://codereview.chromium.org/1632573002/diff/1/tools/gn/function_forward_v...
File tools/gn/function_forward_variables_from.cc (right):

https://codereview.chromium.org/1632573002/diff/1/tools/gn/function_forward_v...
tools/gn/function_forward_variables_from.cc:158: if (args_vector.size() != 2 &&
args_vector.size() != 3) {
On 2016/01/25 at 12:15:25, tfarina wrote:
> just args_vector.size() != 3 right?

No, the last argument is optional, so "forward_variables_from(invoker, "*")" is
still valid for example.

https://codereview.chromium.org/1632573002/diff/1/tools/gn/function_forward_v...
tools/gn/function_forward_variables_from.cc:160: "Expecting exactly two.");
On 2016/01/25 at 12:15:25, tfarina wrote:
> update this error message?

Done, thank you.

[brettw, 2016-01-25 20:44:42]

lgtm

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
File tools/gn/function_forward_variables_from.cc (right):

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:76: " 
forward_variables_from(from_scope, variable_list_or_star,\n"
Check out the help for rebase_path, I used C++-style syntax for documenting the
default variables, and we should probably be consistent.

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:137: "  # This is a template around
another template that uses a variable to\n"
How about "A template that wraps another. It adds behavior based on one
variable, and forwards all others to the nested target."

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:201:
exclusion_set.insert(exclusion_set.end(), cur.string_value());
I think you can just delete the first parameter.

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/scope.cc
File tools/gn/scope.cc (right):

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/scope.cc#newco...
tools/gn/scope.cc:32: mark_dest_used(false) {}
Can you put the } on the following line like the other functions in here?

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/scope.cc#newco...
tools/gn/scope.cc:261: if (!options.excluded_values.empty() &&
This loop is getting a bit out of control with pair.first. Can you help clean it
up by adding at the top of the loop something like:
  const StringPiece& current_name(pair.first);
and use it in all the places in the loop where pair.first appears?

[sdefresne, 2016-01-26 11:08:47]

Patchset #3 (id:40001) has been deleted

[sdefresne, 2016-01-26 11:16:50]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-01-26 11:16:51]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1632573002/#ps60001
(title: "Address comments and expand filter to templates/target_defaults in
NonRecursiveMergeTo")

[commit-bot: I haz the power, 2016-01-26 11:16:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632573002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632573002/60001

[commit-bot: I haz the power, 2016-01-26 11:34:56]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-01-26 11:35:43]

Description was changed from

==========
Support for excluding variable from forwarding via forward_variables_form.

Some templates wants to forward all their parameters to the underlying
target except a select few they use. Previously they had to list all
the variables the underlying target supported which was error prone and
brittle if the underlying target is changed.

With this change they can just use the following:

  forward_variables_from(invoker, "*", ["my_extra_variable"])
  # my_extra_variable is not forwarded, all other variables are.

BUG=580293
==========

to

==========
Support for excluding variable from forwarding via forward_variables_form.

Some templates wants to forward all their parameters to the underlying
target except a select few they use. Previously they had to list all
the variables the underlying target supported which was error prone and
brittle if the underlying target is changed.

With this change they can just use the following:

  forward_variables_from(invoker, "*", ["my_extra_variable"])
  # my_extra_variable is not forwarded, all other variables are.

BUG=580293

Committed: https://crrev.com/bf5ff79020d15e683a12b45be08200ab7d857550
Cr-Commit-Position: refs/heads/master@{#371492}
==========

[commit-bot: I haz the power, 2016-01-26 11:35:44]

Patchset 3 (id:??) landed as
https://crrev.com/bf5ff79020d15e683a12b45be08200ab7d857550
Cr-Commit-Position: refs/heads/master@{#371492}

[sdefresne, 2016-01-26 11:59:44]

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
File tools/gn/function_forward_variables_from.cc (right):

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:76: " 
forward_variables_from(from_scope, variable_list_or_star,\n"
On 2016/01/25 at 20:44:41, brettw wrote:
> Check out the help for rebase_path, I used C++-style syntax for documenting
the default variables, and we should probably be consistent.

Done.

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:137: "  # This is a template around
another template that uses a variable to\n"
On 2016/01/25 at 20:44:41, brettw wrote:
> How about "A template that wraps another. It adds behavior based on one
variable, and forwards all others to the nested target."

Done.

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:201:
exclusion_set.insert(exclusion_set.end(), cur.string_value());
On 2016/01/25 at 20:44:41, brettw wrote:
> I think you can just delete the first parameter.

Done.

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/scope.cc
File tools/gn/scope.cc (right):

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/scope.cc#newco...
tools/gn/scope.cc:32: mark_dest_used(false) {}
On 2016/01/25 at 20:44:41, brettw wrote:
> Can you put the } on the following line like the other functions in here?

Done here an below.

https://codereview.chromium.org/1632573002/diff/20001/tools/gn/scope.cc#newco...
tools/gn/scope.cc:261: if (!options.excluded_values.empty() &&
On 2016/01/25 at 20:44:41, brettw wrote:
> This loop is getting a bit out of control with pair.first. Can you help clean
it up by adding at the top of the loop something like:
>   const StringPiece& current_name(pair.first);
> and use it in all the places in the loop where pair.first appears?

Done.