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

tools/gn/analyzer.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "tools/gn/analyzer.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 #include <vector>
@@ skipping 25 matching lines @@
   std::vector<Label> test_vec;
   bool compile_included_all;
   SourceFileSet source_files;
   LabelSet compile_labels;
   LabelSet test_labels;
 };
 
 struct Outputs {
   std::string status;
   std::string error;
+  bool compile_includes_all;
   LabelSet compile_labels;
   LabelSet test_labels;
   LabelSet invalid_labels;
 };
 
 LabelSet LabelsFor(const TargetSet& targets) {
   LabelSet labels;
   for (const auto& target : targets)
     labels.insert(target->label());
   return labels;
@@ skipping 140 matching lines @@
                           const Label& default_toolchain, Err *err) {
   std::string output;
   auto value = base::MakeUnique<base::DictionaryValue>();
 
   if (outputs.error.size()) {
     WriteString(*value, "error", outputs.error);
     WriteLabels(default_toolchain, *value, "invalid_targets",
                 outputs.invalid_labels);
   } else {
     WriteString(*value, "status", outputs.status);
-    WriteLabels(default_toolchain, *value, "compile_targets",
-                outputs.compile_labels);
+    if (outputs.compile_includes_all) {
+      auto compile_targets = base::WrapUnique(new base::ListValue());

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

For better or worse, I think we have decided to prefer `=
base::MakeUnique<base::ListValue>()` instead of `WrapUnique(new )`

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

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.

+      compile_targets->AppendString("all");
+      value->Set("compile_targets", std::move(compile_targets));
+    } else {
+      WriteLabels(default_toolchain, *value, "compile_targets",
+                  outputs.compile_labels);
+    }
     WriteLabels(default_toolchain, *value, "test_targets", outputs.test_labels);
   }
 
   if (!base::JSONWriter::Write(*value.get(), &output))
     *err = Err(Location(), "Failed to marshal JSON value for output");
   return output;
 }
 
 }  // namespace
 
 Analyzer::Analyzer(const Builder& builder)
     : all_targets_(builder.GetAllResolvedTargets()),
       default_toolchain_(builder.loader()->GetDefaultToolchain()) {
   for (const auto* target : all_targets_) {
     labels_to_targets_[target->label()] = target;
     for (const auto& dep_pair : target->GetDeps(Target::DEPS_ALL))
       dep_map_.insert(std::make_pair(dep_pair.ptr, target));
   }
   for (const auto* target : all_targets_) {
     if (dep_map_.find(target) == dep_map_.end())
       roots_.insert(target);
   }
 }
 
 Analyzer::~Analyzer() {}
 
 std::string Analyzer::Analyze(const std::string& input, Err* err) const {
-  Inputs inputs;
-  Outputs outputs;
+  Inputs inputs = Inputs();
+  Outputs outputs = Outputs();

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

Is this the best way to make sure everything is zero/value-initialized?

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

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.

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

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!

 
   Err local_err = JSONToInputs(default_toolchain_, input, &inputs);
   if (local_err.has_error()) {
     outputs.error = local_err.message();
     return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
   LabelSet invalid_labels;
   for (const auto& label : InvalidLabels(inputs.compile_labels))
     invalid_labels.insert(label);
   for (const auto& label : InvalidLabels(inputs.test_labels))
     invalid_labels.insert(label);
   if (!invalid_labels.empty()) {
     outputs.error = "Invalid targets";
     outputs.invalid_labels = invalid_labels;
     return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
   // TODO(crbug.com/555273): We can do smarter things when we detect changes
   // to build files. For example, if all of the ninja files are unchanged,
   // we know that we can ignore changes to these files. Also, for most .gn
   // files, we can treat a change as simply affecting every target, config,
   // or toolchain defined in that file.
   if (AnyBuildFilesWereModified(inputs.source_files)) {
     outputs.status = "Found dependency (all)";
-    outputs.compile_labels = inputs.compile_labels;
+    if (inputs.compile_included_all) {
+      outputs.compile_includes_all = true;
+    } else {
+      for (const auto label: inputs.compile_labels)
+        outputs.compile_labels.insert(label);
+      for (const auto label: inputs.test_labels)
+        outputs.compile_labels.insert(label);

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

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:20:44]

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]

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

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

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]

ok, done.

+    }
     outputs.test_labels = inputs.test_labels;
     return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
   TargetSet affected_targets = AllAffectedTargets(inputs.source_files);
   if (affected_targets.empty()) {
     outputs.status = "No dependency";
     return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
@@ skipping 115 matching lines @@
 void Analyzer::AddAllRefsTo(const Target* target, TargetSet* results) const {
   if (results->find(target) != results->end())
     return;  // Already found this target.
   results->insert(target);
 
   auto dep_begin = dep_map_.lower_bound(target);
   auto dep_end = dep_map_.upper_bound(target);
   for (auto cur_dep = dep_begin; cur_dep != dep_end; cur_dep++)
     AddAllRefsTo(cur_dep->second, results);
 }