Add not_needed function

Add not_needed function

This can be used to mark variables from a scope as unused, which
means you will not get an error about unused variables for these.

Review-Url: https://codereview.chromium.org/2936923002
Cr-Commit-Position: refs/heads/master@{#483290}
Committed: https://chromium.googlesource.com/chromium/src/+/49be032f4d3274a71b61c0a92f9f747a4b3c90dd

[Petr Hosek, 2017-06-13 19:38:02]

Description was changed from

==========
Add assert_unused function

This can be used to mark variables from a scope as unused and check
that they aren't being used in the current scope.
==========

to

==========
Add assert_unused function

This can be used to mark variables from a scope as unused and check
that they aren't being used in the current scope.
==========

[Petr Hosek, 2017-06-13 19:38:03]

phosek@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[brettw, 2017-06-20 17:27:26]

Sorry I'm so behind!

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

https://codereview.chromium.org/2936923002/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:645: "not_needed: Mark variables from scope as unusedd.";
"unusedd" spelling

https://codereview.chromium.org/2936923002/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:659: not_needed(invoker, [ "data_deps", "deps" ])
Can you add support for a third form that modifies the current scope? I think we
can do this by just marking the first argument as optional. This should be easy
to tell by what types things are.
  not_needed([ "config" ])
  not_needed("*", [ "config" ])
We probably don't need the "*" form for this, but then I think it gets too
complicated to explain, so may as well keep it.

The need for this comes up in legitimate cases, especially in the Android
templates which have insane conditionals and its' really difficult to tell when
things will be needed in advance:

https://cs.chromium.org/chromium/src/build/config/android/internal_rules.gni?...

https://cs.chromium.org/chromium/src/build/config/android/internal_rules.gni?...

To support this case, I think we can remove the concept of "unusable' variables.
There can be nice uses of this, but it does prevent others like the Android
ones, and I'm not sure the practical value is so great.

I think then you can remove some of the out err values for the Scope functions
which will make this patch smaller.

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

https://codereview.chromium.org/2936923002/diff/20001/tools/gn/scope.h#newcod...
tools/gn/scope.h:212: void MarkAllUnusable(Err* err, std::set<std::string>
excluded_values);
The set should be passed by const ref.

[Petr Hosek, 2017-06-21 02:17:40]

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

https://codereview.chromium.org/2936923002/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:645: "not_needed: Mark variables from scope as unusedd.";
On 2017/06/20 17:27:25, brettw wrote:
> "unusedd" spelling

Done.

https://codereview.chromium.org/2936923002/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:659: not_needed(invoker, [ "data_deps", "deps" ])
On 2017/06/20 17:27:25, brettw wrote:
> Can you add support for a third form that modifies the current scope? I think
we
> can do this by just marking the first argument as optional. This should be
easy
> to tell by what types things are.
>   not_needed([ "config" ])
>   not_needed("*", [ "config" ])
> We probably don't need the "*" form for this, but then I think it gets too
> complicated to explain, so may as well keep it.
> 
> The need for this comes up in legitimate cases, especially in the Android
> templates which have insane conditionals and its' really difficult to tell
when
> things will be needed in advance:
> 
>
https://cs.chromium.org/chromium/src/build/config/android/internal_rules.gni?...
> 
>
https://cs.chromium.org/chromium/src/build/config/android/internal_rules.gni?...
> 
> To support this case, I think we can remove the concept of "unusable'
variables.
> There can be nice uses of this, but it does prevent others like the Android
> ones, and I'm not sure the practical value is so great.
> 
> I think then you can remove some of the out err values for the Scope functions
> which will make this patch smaller.

Done.

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

https://codereview.chromium.org/2936923002/diff/20001/tools/gn/scope.h#newcod...
tools/gn/scope.h:212: void MarkAllUnusable(Err* err, std::set<std::string>
excluded_values);
On 2017/06/20 17:27:25, brettw wrote:
> The set should be passed by const ref.

Done.

[Petr Hosek, 2017-06-24 02:05:13]

Description was changed from

==========
Add assert_unused function

This can be used to mark variables from a scope as unused and check
that they aren't being used in the current scope.
==========

to

==========
Add not_needed function

This can be used to mark variables from a scope as unused and check
that they aren't being used in the current scope.
==========

[Petr Hosek, 2017-06-28 18:45:58]

ping

[brettw, 2017-06-28 20:16:49]

Sorry for the delay as usual :/

LGTM

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/args.cc
File tools/gn/args.cc (right):

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/args.cc#newcode9
tools/gn/args.cc:9: #include "tools/gn/scheduler.h"
This should be no longer necessary.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:654: you will not get an error about unused variables for
these.
Can you make more explicit here that variable_to_ignore_list allows excluding
variables from "all matches" if the variable list is "*". The current best
description of "variables to ignore as not needed" gets pretty confusing.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:734: } else {
You can just do "else if" here and avoid the extra indenting.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:735: if (value->type() == Value::LIST) {
Can you throw an error here if an exclusion list is specified? Otherwise this
will be silently ignored which may be surprising.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions_targ...
File tools/gn/functions_target_unittest.cc (right):

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions_targ...
tools/gn/functions_target_unittest.cc:59: "not_needed(a, \"*\")\n");
Can you test the exclusion list also?

[Petr Hosek, 2017-06-28 23:21:32]

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/args.cc
File tools/gn/args.cc (right):

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/args.cc#newcode9
tools/gn/args.cc:9: #include "tools/gn/scheduler.h"
On 2017/06/28 20:16:49, brettw wrote:
> This should be no longer necessary.

Done.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:654: you will not get an error about unused variables for
these.
On 2017/06/28 20:16:49, brettw wrote:
> Can you make more explicit here that variable_to_ignore_list allows excluding
> variables from "all matches" if the variable list is "*". The current best
> description of "variables to ignore as not needed" gets pretty confusing.

Done.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:734: } else {
On 2017/06/28 20:16:49, brettw wrote:
> You can just do "else if" here and avoid the extra indenting.

Done.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:735: if (value->type() == Value::LIST) {
On 2017/06/28 20:16:49, brettw wrote:
> Can you throw an error here if an exclusion list is specified? Otherwise this
> will be silently ignored which may be surprising.

Done.

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions_targ...
File tools/gn/functions_target_unittest.cc (right):

https://codereview.chromium.org/2936923002/diff/40001/tools/gn/functions_targ...
tools/gn/functions_target_unittest.cc:59: "not_needed(a, \"*\")\n");
On 2017/06/28 20:16:49, brettw wrote:
> Can you test the exclusion list also?

Done.

[Petr Hosek, 2017-06-29 02:22:10]

Description was changed from

==========
Add not_needed function

This can be used to mark variables from a scope as unused and check
that they aren't being used in the current scope.
==========

to

==========
Add not_needed function

This can be used to mark variables from a scope as unused, which
means you will not get an error about unused variables for these.
==========

[Petr Hosek, 2017-06-29 02:22:17]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2017-06-29 02:22:18]

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/2936923002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-29 02:22:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 04:06:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498702937309280,
"parent_rev": "4ef08e4c5ad81f0f285054df0d705258178c4ede", "commit_rev":
"49be032f4d3274a71b61c0a92f9f747a4b3c90dd"}

[commit-bot: I haz the power, 2017-06-29 04:06:52]

Description was changed from

==========
Add not_needed function

This can be used to mark variables from a scope as unused, which
means you will not get an error about unused variables for these.
==========

to

==========
Add not_needed function

This can be used to mark variables from a scope as unused, which
means you will not get an error about unused variables for these.

Review-Url: https://codereview.chromium.org/2936923002
Cr-Commit-Position: refs/heads/master@{#483290}
Committed:
https://chromium.googlesource.com/chromium/src/+/49be032f4d3274a71b61c0a92f9f...
==========

[commit-bot: I haz the power, 2017-06-29 04:06:53]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/49be032f4d3274a71b61c0a92f9f...