Fix an issue in `gn analyze` when building all.

Fix an issue in `gn analyze` when building all.

If you ran `gn analyze` on a list of files that changed a build file,
the list of compile targets and output targets should've been returned
unchanged, but if the additional_compile_targets input included "all", the
returned list was left empty.

R=brettw@chromium.org
BUG=648532
Committed: https://crrev.com/385a310b530199122f9781ead9410afe09b9c8d2
Cr-Commit-Position: refs/heads/master@{#419866}

[Dirk Pranke, 2016-09-20 18:53:38]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2016-09-20 18:54:23]

dpranke@chromium.org changed reviewers:
+ thakis@chromium.org

[Dirk Pranke, 2016-09-20 18:54:23]

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

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:243: Outputs outputs = Outputs();
Is this the best way to make sure everything is zero/value-initialized?

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:275: outputs.compile_labels.insert(label);
This is the thing that is equivalent to a set_union, but I couldn't figure out
how to the assignment operator happy; it seemed to want weird things to be
const. 

Let me know if you want me to keep playing with this, but I'd rather land a fix
first to avoid further incorrect builds.

[Nico, 2016-09-20 19:16:59]

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

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:243: Outputs outputs = Outputs();
On 2016/09/20 18:54:23, Dirk Pranke wrote:
> Is this the best way to make sure everything is zero/value-initialized?

The best way is to give your thing a ctor, eg by doing

  struct Outputs {
    ...
    bool includes_all = false;
    ...
  };

Then you don't need to be careful everytime you make a variable of this type.

[Nico, 2016-09-20 19:20:44]

Makes sense to me, but if you want someone who actually knows gn code, wait for
Brett :-) Else, lgtm.

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

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:208: auto compile_targets = base::WrapUnique(new
base::ListValue());
For better or worse, I think we have decided to prefer `=
base::MakeUnique<base::ListValue>()` instead of `WrapUnique(new )`

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:275: outputs.compile_labels.insert(label);
On 2016/09/20 18:54:23, Dirk Pranke wrote:
> This is the thing that is equivalent to a set_union, but I couldn't figure out
> how to the assignment operator happy; it seemed to want weird things to be
> const. 
> 
> Let me know if you want me to keep playing with this, but I'd rather land a
fix
> first to avoid further incorrect builds.

I haven't looked at the docs of std::set, but I'd've expected something like
this to work:

  outputs.compile_labels.isert(inputs.compile_labels.begin(),
inputs.compile_labels.end());
  outputs.compile_labels.insert(inputs.test_labels.begin(),
inputs.test_labels.end());

[Dirk Pranke, 2016-09-20 19:24:17]

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

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:208: auto compile_targets = base::WrapUnique(new
base::ListValue());
On 2016/09/20 19:20:44, Nico wrote:
> For better or worse, I think we have decided to prefer `=
> base::MakeUnique<base::ListValue>()` instead of `WrapUnique(new )`

I think we mostly use WrapUnique() elsewhere, still, so I'd probably leave this
as-is for now and defer cleanup to a subsequent change, unless brettw@ also
wants me to change this now.

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:243: Outputs outputs = Outputs();
On 2016/09/20 19:16:59, Nico wrote:
> On 2016/09/20 18:54:23, Dirk Pranke wrote:
> > Is this the best way to make sure everything is zero/value-initialized?
> 
> The best way is to give your thing a ctor, eg by doing
> 
>   struct Outputs {
>     ...
>     bool includes_all = false;
>     ...
>   };
> 
> Then you don't need to be careful everytime you make a variable of this type.

Ah, I was wondering if you could do something like that, but couldn't find any
examples offhand and failed to think to just try it. Thanks!

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:275: outputs.compile_labels.insert(label);
On 2016/09/20 19:20:44, Nico wrote:
> On 2016/09/20 18:54:23, Dirk Pranke wrote:
> > This is the thing that is equivalent to a set_union, but I couldn't figure
out
> > how to the assignment operator happy; it seemed to want weird things to be
> > const. 
> > 
> > Let me know if you want me to keep playing with this, but I'd rather land a
> fix
> > first to avoid further incorrect builds.
> 
> I haven't looked at the docs of std::set, but I'd've expected something like
> this to work:
> 
>   outputs.compile_labels.isert(inputs.compile_labels.begin(),
> inputs.compile_labels.end());
>   outputs.compile_labels.insert(inputs.test_labels.begin(),
> inputs.test_labels.end());

Yeah, that might also work, though I'm not sure that either that or set_union()
is actually clearer than what I've written.

[Dirk Pranke, 2016-09-20 19:24:24]

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

[commit-bot: I haz the power, 2016-09-20 19:24:59]

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

[brettw, 2016-09-20 19:28:53]

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

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:275: outputs.compile_labels.insert(label);
Just do what Nico suggested. Since you already have a set, a union is trivial. I
realize now that the problem with your old code was using a set as the output
iterator. set_union is trying to write things out in order, while the
destination container is also thinking it has control over the order.

[Dirk Pranke, 2016-09-20 19:36:20]

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

https://codereview.chromium.org/2350963006/diff/20001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:275: outputs.compile_labels.insert(label);
On 2016/09/20 19:28:53, brettw (ping on IM after 24h) wrote:
> Just do what Nico suggested. Since you already have a set, a union is trivial.
I
> realize now that the problem with your old code was using a set as the output
> iterator. set_union is trying to write things out in order, while the
> destination container is also thinking it has control over the order.

ok, done.

[Dirk Pranke, 2016-09-20 19:36:28]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-09-20 19:36:28]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2350963006/#ps60001
(title: "use nico's union code")

[commit-bot: I haz the power, 2016-09-20 19:37:00]

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-09-20 20:17:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 20:17:50]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dirk Pranke, 2016-09-20 20:28:48]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-09-20 20:29:08]

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-09-20 20:48:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 20:48:47]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Dirk Pranke, 2016-09-20 20:55:40]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-09-20 20:56:10]

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-09-20 22:04:28]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-20 22:06:33]

Description was changed from

==========
Fix an issue in `gn analyze` when building all.

If you ran `gn analyze` on a list of files that changed a build file,
the list of compile targets and output targets should've been returned
unchanged, but if the additional_compile_targets input included "all", the
returned list was left empty.

R=brettw@chromium.org
BUG=648532
==========

to

==========
Fix an issue in `gn analyze` when building all.

If you ran `gn analyze` on a list of files that changed a build file,
the list of compile targets and output targets should've been returned
unchanged, but if the additional_compile_targets input included "all", the
returned list was left empty.

R=brettw@chromium.org
BUG=648532

Committed: https://crrev.com/385a310b530199122f9781ead9410afe09b9c8d2
Cr-Commit-Position: refs/heads/master@{#419866}
==========

[commit-bot: I haz the power, 2016-09-20 22:06:34]

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