Makes declared_arguments per toolchain

Makes declared_arguments per toolchain

This means each toolchain can declare arguments as it sees fit. Prior to this change each toolchain needed to declare the argument at exactly the same place, which is not always possible. For example, the following triggered an error:

declare_args() {
  foo = 1
  if (xxx) {
    foo = 2
  }

with xxx being specific to one toolchain.

TEST=covered by test
BUG=465029
R=brettw@chromium.org

Committed: https://crrev.com/ca5ccceffa70d6a4e20cacaa12c44e4e9d0ebf2c
Cr-Commit-Position: refs/heads/master@{#320753}

[sky, 2015-03-13 20:43:13]

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

https://codereview.chromium.org/984353002/diff/120001/tools/gn/args.h#newcode75
tools/gn/args.h:75: static bool VerifyAllOverridesUsed(
I moved this to the private block as no one but this class used it and I was
worried it may be misused externally.

[sky, 2015-03-13 20:43:46]

I updated this as discussed. Args now stores declared arguments in a toolchain
specific map.

[brettw, 2015-03-14 05:14:32]

Thanks, just need to work on the override checking.

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

https://codereview.chromium.org/984353002/diff/120001/tools/gn/args.cc#newcod...
tools/gn/args.cc:175: if (!VerifyAllOverridesUsed(all_overrides_,
map_pair.second, err))
I don't think this is quite right. We want to return true if all overrides were
used anywhere, not whether they were all used in all toolchains.

One way is to make a copy of the overrides (generally very small), go through
each toolchain and remove the ones we find in that toolchain, and then throw an
error listing all remaining items.

We should probably add a case like this to the test.

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

https://codereview.chromium.org/984353002/diff/120001/tools/gn/args.h#newcode91
tools/gn/args.h:91: // NOTE: this does not grab a lock.
I was calling such functions ...Locked. I think if you rename it you can delete
this comment.

[brettw, 2015-03-14 05:15:04]

Also, need a better commit message.

[sky, 2015-03-16 16:20:45]

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

https://codereview.chromium.org/984353002/diff/120001/tools/gn/args.cc#newcod...
tools/gn/args.cc:175: if (!VerifyAllOverridesUsed(all_overrides_,
map_pair.second, err))
On 2015/03/14 05:14:32, brettw wrote:
> I don't think this is quite right. We want to return true if all overrides
were
> used anywhere, not whether they were all used in all toolchains.
> 
> One way is to make a copy of the overrides (generally very small), go through
> each toolchain and remove the ones we find in that toolchain, and then throw
an
> error listing all remaining items.
> 
> We should probably add a case like this to the test.

Done.

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

https://codereview.chromium.org/984353002/diff/120001/tools/gn/args.h#newcode91
tools/gn/args.h:91: // NOTE: this does not grab a lock.
On 2015/03/14 05:14:32, brettw wrote:
> I was calling such functions ...Locked. I think if you rename it you can
delete
> this comment.

Done.

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

https://codereview.chromium.org/984353002/diff/140001/tools/gn/args.cc#newcod...
tools/gn/args.cc:213: "\" was set as a build argument\nbut never appeared in a "
+
git cl format did this. No idea why it indented here, but I left it rather than
fight it.

[brettw, 2015-03-16 17:19:23]

lgtm

[sky, 2015-03-16 17:27:02]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2015-03-16 17:27:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/984353002/140001

[commit-bot: I haz the power, 2015-03-16 18:06:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-16 18:07:00]

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