GN: Don't define argument overrides globally

GN: Don't define argument overrides globally

Previously, argument overrides (set via "gn args" or toolchain_args()) were
global, even though their default values (set inside declare_args()) were set
locally and thus only available to the file that set them, plus any file that
imported it.

Now, argument overrides are always applied locally, at the end of the
declare_args() block. This ensures that argument defaults and overrides
behave in the same way.

BUG=619963
Committed: https://crrev.com/5662a8082448584fd3d77f0dba03759b2163e8e5
Cr-Commit-Position: refs/heads/master@{#404746}

[timn, 2016-06-22 23:35:49]

tim@rnc-ag.de changed reviewers:
+ brettw@chromium.org

[brettw, 2016-06-23 22:51:44]

What's the reason this doesn't apply to toolchain overrides? It would be nice if
the description went into more detail here.

Sorry I haven't gotten to actually reviewing the code yet...

[brettw, 2016-06-23 23:09:01]

I'm remembering this more now. The toolchain issue is what stopped me from
fixing this before. I do think we should fix that, since toolchain_args being
global but other ones not is even more confusing than what we have now.

Can we somehow register the toolchain args for the toolchain label in this file
when we know them, and then have this look up the right ones to use when
applying the overrides?

[timn, 2016-06-23 23:20:14]

On 2016/06/23 23:09:01, brettw wrote:
> I'm remembering this more now. The toolchain issue is what stopped me from
> fixing this before. I do think we should fix that, since toolchain_args being
> global but other ones not is even more confusing than what we have now.
> 
> Can we somehow register the toolchain args for the toolchain label in this
file
> when we know them, and then have this look up the right ones to use when
> applying the overrides?

Yes, declarations are already per-toolchain, so this should be relatively easy
to add.
I'll look into it now.

[timn, 2016-06-24 00:09:08]

toolchain overrides are now supported as well.

[timn, 2016-06-24 00:14:28]

Description was changed from

==========
GN: Don't define argument overrides globally

Previously, arguments set via "gn args" were global, even though their default
values (set inside declare_args()) were only set locally and thus only
available to the file that set them, plus any file that imported it.

Now, the argument is always defined locally, at the time it appears inside the
declare_args() block.

Note that this doesn't apply to toolchain_args() overrides, which are still
global.

BUG=619963
==========

to

==========
GN: Don't define argument overrides globally

Previously, argument overrides (set via "gn args" or toolchain_args()) were
global, even though their default values (set inside declare_args()) were set
locally and thus only available to the file that set them, plus any file that
imported it.

Now, argument overrides are always applied locally, at the end of the
declare_args() block. This ensures that argument defaults and overrides
behave in the same way.

BUG=619963
==========

[brettw, 2016-07-04 01:01:40]

Sorry this fell off my list, will get to it after the US holiday.

[brettw, 2016-07-11 19:42:30]

Thanks! Sorry this took so long. I have some minor comments before checkin. LGTM
with these.

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

https://codereview.chromium.org/2092623002/diff/20001/tools/gn/args.cc#newcod...
tools/gn/args.cc:203: override->second.origin());
I feel like these the toolchain overrides and regular overrides cases should be
symmetrical since they're different cases of the same thing. So can you either
delete the "continue" above and put the rest of this code in an "else" block, or
add a "continue" here and remove the "else" block for the fall-through case
below?

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

https://codereview.chromium.org/2092623002/diff/20001/tools/gn/args.h#newcode43
tools/gn/args.h:43: // flags, then any overrides stored in this object, then
applies any
This comment probably needs to be updated, since it now saves instead of applies
the toolchain overrides. Probably best to say something like it saves the
toolchain overrides so they can be applied to declare_args blocks that appear
when loading files in that toolchain.

[brettw, 2016-07-11 21:49:33]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 21:50:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[timn, 2016-07-11 22:29:56]

The CQ bit was checked by tim@rnc-ag.de

[timn, 2016-07-11 22:29:56]

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/2092623002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2016-07-11 22:30:55]

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, 2016-07-12 00:08:56]

Description was changed from

==========
GN: Don't define argument overrides globally

Previously, argument overrides (set via "gn args" or toolchain_args()) were
global, even though their default values (set inside declare_args()) were set
locally and thus only available to the file that set them, plus any file that
imported it.

Now, argument overrides are always applied locally, at the end of the
declare_args() block. This ensures that argument defaults and overrides
behave in the same way.

BUG=619963
==========

to

==========
GN: Don't define argument overrides globally

Previously, argument overrides (set via "gn args" or toolchain_args()) were
global, even though their default values (set inside declare_args()) were set
locally and thus only available to the file that set them, plus any file that
imported it.

Now, argument overrides are always applied locally, at the end of the
declare_args() block. This ensures that argument defaults and overrides
behave in the same way.

BUG=619963
==========

[commit-bot: I haz the power, 2016-07-12 00:08:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-12 00:09:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 00:11:33]

Description was changed from

==========
GN: Don't define argument overrides globally

Previously, argument overrides (set via "gn args" or toolchain_args()) were
global, even though their default values (set inside declare_args()) were set
locally and thus only available to the file that set them, plus any file that
imported it.

Now, argument overrides are always applied locally, at the end of the
declare_args() block. This ensures that argument defaults and overrides
behave in the same way.

BUG=619963
==========

to

==========
GN: Don't define argument overrides globally

Previously, argument overrides (set via "gn args" or toolchain_args()) were
global, even though their default values (set inside declare_args()) were set
locally and thus only available to the file that set them, plus any file that
imported it.

Now, argument overrides are always applied locally, at the end of the
declare_args() block. This ensures that argument defaults and overrides
behave in the same way.

BUG=619963

Committed: https://crrev.com/5662a8082448584fd3d77f0dba03759b2163e8e5
Cr-Commit-Position: refs/heads/master@{#404746}
==========

[commit-bot: I haz the power, 2016-07-12 00:11:34]

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