Convert GN's deps iterator to work with range-based for loops.

Reworks DepsIterator so it is compatible with STL iterators enough to work with range-based for loops. The iterator is now created by a target rather than taking a target as an argument, which makes the loops more natural.

I also changed some loops around code I was touching to use range-based.

Review URL: https://codereview.chromium.org/610043002

Cr-Commit-Position: refs/heads/master@{#297122}

11 files changed, 138 insertions, 112 deletions

diff --git a/tools/gn/binary_target_generator.cc b/tools/gn/binary_target_generator.cc
index 73f7cd2..d73c76e 100644
--- a/tools/gn/binary_target_generator.cc
+++ b/tools/gn/binary_target_generator.cc
@@ -120,9 +120,8 @@ bool BinaryTargetGenerator::FillAllowCircularIncludesFrom() {
   // Validate that all circular includes entries are in the deps.
   for (size_t circular_i = 0; circular_i < circular.size(); circular_i++) {
     bool found_dep = false;
-    for (DepsIterator iter(target_, DepsIterator::LINKED_ONLY);
-         !iter.done(); iter.Advance()) {
-      if (iter.label() == circular[circular_i]) {
+    for (const auto& dep_pair : target_->GetDeps(Target::DEPS_LINKED)) {
+      if (dep_pair.label == circular[circular_i]) {
         found_dep = true;
         break;
       }
diff --git a/tools/gn/builder.cc b/tools/gn/builder.cc
index 6198777..e92a7e0 100644
--- a/tools/gn/builder.cc
+++ b/tools/gn/builder.cc
@@ -481,14 +481,13 @@ bool Builder::ResolveForwardDependentConfigs(Target* target, Err* err) {
 
   // Assume that the lists are small so that brute-force n^2 is appropriate.
   for (size_t config_i = 0; config_i < configs.size(); config_i++) {
-    for (DepsIterator dep_iter(target, DepsIterator::LINKED_ONLY);
-         !dep_iter.done(); dep_iter.Advance()) {
-      if (configs[config_i].label == dep_iter.label()) {
-        DCHECK(dep_iter.target());  // Should already be resolved.
+    for (const auto& dep_pair : target->GetDeps(Target::DEPS_LINKED)) {
+      if (configs[config_i].label == dep_pair.label) {
+        DCHECK(dep_pair.ptr);  // Should already be resolved.
         // UniqueVector's contents are constant so uniqueness is preserved, but
         // we want to update this pointer which doesn't change uniqueness
         // (uniqueness in this vector is determined by the label only).
-        const_cast<LabelTargetPair&>(configs[config_i]).ptr = dep_iter.target();
+        const_cast<LabelTargetPair&>(configs[config_i]).ptr = dep_pair.ptr;
         break;
       }
     }
diff --git a/tools/gn/command_desc.cc b/tools/gn/command_desc.cc
index ccce001e..d43577e 100644
--- a/tools/gn/command_desc.cc
+++ b/tools/gn/command_desc.cc
@@ -51,8 +51,8 @@ void RecursiveCollectDeps(const Target* target, std::set<Label>* result) {
 }
 
 void RecursiveCollectChildDeps(const Target* target, std::set<Label>* result) {
-  for (DepsIterator iter(target); !iter.done(); iter.Advance())
-    RecursiveCollectDeps(iter.target(), result);
+  for (const auto& pair : target->GetDeps(Target::DEPS_ALL))
+    RecursiveCollectDeps(pair.ptr, result);
 }
 
 // Prints dependencies of the given target (not the target itself). If the
@@ -65,8 +65,8 @@ void RecursivePrintDeps(const Target* target,
                         int indent_level) {
   // Combine all deps into one sorted list.
   std::vector<LabelTargetPair> sorted_deps;
-  for (DepsIterator iter(target); !iter.done(); iter.Advance())
-    sorted_deps.push_back(iter.pair());
+  for (const auto& pair : target->GetDeps(Target::DEPS_ALL))
+    sorted_deps.push_back(pair);
   std::sort(sorted_deps.begin(), sorted_deps.end(),
             LabelPtrLabelLess<Target>());
 
@@ -140,8 +140,8 @@ void PrintDeps(const Target* target, bool display_header) {
           "\nDirect dependencies "
           "(try also \"--all\", \"--tree\", or even \"--all --tree\"):\n");
     }
-    for (DepsIterator iter(target); !iter.done(); iter.Advance())
-      deps.push_back(iter.label());
+    for (const auto& pair : target->GetDeps(Target::DEPS_ALL))
+      deps.push_back(pair.label);
   }
 
   std::sort(deps.begin(), deps.end());
diff --git a/tools/gn/command_refs.cc b/tools/gn/command_refs.cc
index a1b37e5..c55726b 100644
--- a/tools/gn/command_refs.cc
+++ b/tools/gn/command_refs.cc
@@ -27,12 +27,9 @@ typedef std::multimap<const Target*, const Target*> DepMap;
 
 // Populates the reverse dependency map for the targets in the Setup.
 void FillDepMap(Setup* setup, DepMap* dep_map) {
-  std::vector<const Target*> targets =
-      setup->builder()->GetAllResolvedTargets();
-
-  for (size_t target_i = 0; target_i < targets.size(); target_i++) {
-    for (DepsIterator iter(targets[target_i]); !iter.done(); iter.Advance())
-      dep_map->insert(std::make_pair(iter.target(), targets[target_i]));
+  for (const auto& target : setup->builder()->GetAllResolvedTargets()) {
+    for (const auto& dep_pair : target->GetDeps(Target::DEPS_ALL))
+      dep_map->insert(std::make_pair(dep_pair.ptr, target));
   }
 }
 
diff --git a/tools/gn/deps_iterator.cc b/tools/gn/deps_iterator.cc
index 1f964f3..8bbb760 100644
--- a/tools/gn/deps_iterator.cc
+++ b/tools/gn/deps_iterator.cc
@@ -6,23 +6,22 @@
 
 #include "tools/gn/target.h"
 
-DepsIterator::DepsIterator(const Target* t) : current_index_(0) {
-  vect_stack_[0] = &t->public_deps();
-  vect_stack_[1] = &t->private_deps();
-  vect_stack_[2] = &t->data_deps();
-
-  if (vect_stack_[0]->empty())
-    Advance();
+DepsIterator::DepsIterator() : current_index_(0) {
+  vect_stack_[0] = nullptr;
+  vect_stack_[1] = nullptr;
+  vect_stack_[2] = nullptr;
 }
 
-// Iterate over the public and private linked deps, but not the data deps.
-DepsIterator::DepsIterator(const Target* t, LinkedOnly) : current_index_(0) {
-  vect_stack_[0] = &t->public_deps();
-  vect_stack_[1] = &t->private_deps();
-  vect_stack_[2] = NULL;
+DepsIterator::DepsIterator(const LabelTargetVector* a,
+                           const LabelTargetVector* b,
+                           const LabelTargetVector* c)
+    : current_index_(0) {
+  vect_stack_[0] = a;
+  vect_stack_[1] = b;
+  vect_stack_[2] = c;
 
-  if (vect_stack_[0]->empty())
-    Advance();
+  if (vect_stack_[0] && vect_stack_[0]->empty())
+    operator++();
 }
 
 // Advance to the next position. This assumes there are more vectors.
@@ -30,7 +29,7 @@ DepsIterator::DepsIterator(const Target* t, LinkedOnly) : current_index_(0) {
 // For internal use, this function tolerates an initial index equal to the
 // length of the current vector. In this case, it will advance to the next
 // one.
-void DepsIterator::Advance() {
+DepsIterator& DepsIterator::operator++() {
   DCHECK(vect_stack_[0]);
 
   current_index_++;
@@ -38,11 +37,20 @@ void DepsIterator::Advance() {
     // Advance to next vect. Shift the elements left by one.
     vect_stack_[0] = vect_stack_[1];
     vect_stack_[1] = vect_stack_[2];
-    vect_stack_[2] = NULL;
+    vect_stack_[2] = nullptr;
 
     current_index_ = 0;
 
     if (vect_stack_[0] && vect_stack_[0]->empty())
-      Advance();
+      operator++();
   }
+  return *this;
+}
+
+DepsIteratorRange::DepsIteratorRange(const DepsIterator& b)
+    : begin_(b),
+      end_() {
+}
+
+DepsIteratorRange::~DepsIteratorRange() {
 }
diff --git a/tools/gn/deps_iterator.h b/tools/gn/deps_iterator.h
index 5fc3c01e..1c3a75e 100644
--- a/tools/gn/deps_iterator.h
+++ b/tools/gn/deps_iterator.h
@@ -10,52 +10,64 @@
 
 class Target;
 
-// Iterates over the deps of a target.
+// Provides an iterator for iterating over multiple LabelTargetVectors to
+// make it convenient to iterate over all deps of a target.
 //
-// Since there are multiple kinds of deps, this iterator allows looping over
-// each one in one loop.
+// This works by maintaining a simple stack of vectors (since we have a fixed
+// number of deps types). When the stack is empty, we've reached the end. This
+// means that the default-constructed iterator == end() for any sequence.
 class DepsIterator {
  public:
-  enum LinkedOnly {
-    LINKED_ONLY,
-  };
+  // Creates an empty iterator.
+  DepsIterator();
 
-  // Iterate over public, private, and data deps.
-  explicit DepsIterator(const Target* t);
+  // Iterate over the deps in the given vectors. If passing less than three,
+  // pad with nulls.
+  DepsIterator(const LabelTargetVector* a,
+               const LabelTargetVector* b,
+               const LabelTargetVector* c);
 
-  // Iterate over the public and private linked deps, but not the data deps.
-  DepsIterator(const Target* t, LinkedOnly);
-
-  // Returns true when there are no more targets.
-  bool done() const {
-    return !vect_stack_[0];
-  }
-
-  // Advance to the next position. This assumes !done().
+  // Prefix increment operator. This assumes there are more items (i.e.
+  // *this != DepsIterator()).
   //
   // For internal use, this function tolerates an initial index equal to the
   // length of the current vector. In this case, it will advance to the next
   // one.
-  void Advance();
+  DepsIterator& operator++();
 
-  // The current dependency.
-  const LabelTargetPair& pair() const {
+  // Comparison for STL-based loops.
+  bool operator!=(const DepsIterator& other) {
+    return current_index_ != other.current_index_ ||
+        vect_stack_[0] != other.vect_stack_[0] ||
+        vect_stack_[1] != other.vect_stack_[1] ||
+        vect_stack_[2] != other.vect_stack_[2];
+  }
+
+  // Dereference operator for STL-compatible iterators.
+  const LabelTargetPair& operator*() const {
     DCHECK_LT(current_index_, vect_stack_[0]->size());
     return (*vect_stack_[0])[current_index_];
   }
 
-  // The pointer to the current dependency.
-  const Target* target() const { return pair().ptr; }
-
-  // The label of the current dependency.
-  const Label& label() const { return pair().label; }
-
  private:
   const LabelTargetVector* vect_stack_[3];
 
   size_t current_index_;
+};
+
+// Provides a virtual container implementing begin() and end() for a
+// sequence of deps. This can then be used in range-based for loops.
+class DepsIteratorRange {
+ public:
+  explicit DepsIteratorRange(const DepsIterator& b);
+  ~DepsIteratorRange();
+
+  const DepsIterator& begin() const { return begin_; }
+  const DepsIterator& end() const { return end_; }
 
-  DISALLOW_COPY_AND_ASSIGN(DepsIterator);
+ private:
+  DepsIterator begin_;
+  DepsIterator end_;
 };
 
 #endif  // TOOLS_GN_DEPS_ITERATOR_H_
diff --git a/tools/gn/ninja_action_target_writer.cc b/tools/gn/ninja_action_target_writer.cc
index fe62f63..ccbb570 100644
--- a/tools/gn/ninja_action_target_writer.cc
+++ b/tools/gn/ninja_action_target_writer.cc
@@ -32,9 +32,8 @@ void NinjaActionTargetWriter::Run() {
   // operating on the result of that previous step, so we need to be sure to
   // serialize these.
   std::vector<const Target*> extra_hard_deps;
-  for (DepsIterator iter(target_, DepsIterator::LINKED_ONLY);
-       !iter.done(); iter.Advance())
-    extra_hard_deps.push_back(iter.target());
+  for (const auto& pair : target_->GetDeps(Target::DEPS_LINKED))
+    extra_hard_deps.push_back(pair.ptr);
 
   // For ACTIONs this is a bit inefficient since it creates an input dep
   // stamp file even though we're only going to use it once. It would save a
diff --git a/tools/gn/ninja_binary_target_writer.cc b/tools/gn/ninja_binary_target_writer.cc
index 1480f69..ebd9282 100644
--- a/tools/gn/ninja_binary_target_writer.cc
+++ b/tools/gn/ninja_binary_target_writer.cc
@@ -375,26 +375,21 @@ void NinjaBinaryTargetWriter::GetDeps(
     UniqueVector<OutputFile>* extra_object_files,
     UniqueVector<const Target*>* linkable_deps,
     UniqueVector<const Target*>* non_linkable_deps) const {
-  const UniqueVector<const Target*>& inherited =
-      target_->inherited_libraries();
-
   // Normal public/private deps.
-  for (DepsIterator iter(target_, DepsIterator::LINKED_ONLY); !iter.done();
-       iter.Advance()) {
-    ClassifyDependency(iter.target(), extra_object_files,
+  for (const auto& pair : target_->GetDeps(Target::DEPS_LINKED)) {
+    ClassifyDependency(pair.ptr, extra_object_files,
                        linkable_deps, non_linkable_deps);
   }
 
   // Inherited libraries.
-  for (size_t i = 0; i < inherited.size(); i++) {
-    ClassifyDependency(inherited[i], extra_object_files,
+  for (const auto& inherited_target : target_->inherited_libraries()) {
+    ClassifyDependency(inherited_target, extra_object_files,
                        linkable_deps, non_linkable_deps);
   }
 
   // Data deps.
-  const LabelTargetVector& data_deps = target_->data_deps();
-  for (size_t i = 0; i < data_deps.size(); i++)
-    non_linkable_deps->push_back(data_deps[i].ptr);
+  for (const auto& data_dep_pair : target_->data_deps())
+    non_linkable_deps->push_back(data_dep_pair.ptr);
 }
 
 void NinjaBinaryTargetWriter::ClassifyDependency(
diff --git a/tools/gn/ninja_group_target_writer.cc b/tools/gn/ninja_group_target_writer.cc
index c672f26..c298413 100644
--- a/tools/gn/ninja_group_target_writer.cc
+++ b/tools/gn/ninja_group_target_writer.cc
@@ -22,14 +22,13 @@ void NinjaGroupTargetWriter::Run() {
   // A group rule just generates a stamp file with dependencies on each of
   // the deps and data_deps in the group.
   std::vector<OutputFile> output_files;
-  for (DepsIterator iter(target_, DepsIterator::LINKED_ONLY);
-       !iter.done(); iter.Advance())
-    output_files.push_back(iter.target()->dependency_output_file());
+  for (const auto& pair : target_->GetDeps(Target::DEPS_LINKED))
+    output_files.push_back(pair.ptr->dependency_output_file());
 
   std::vector<OutputFile> data_output_files;
   const LabelTargetVector& data_deps = target_->data_deps();
-  for (size_t i = 0; i < data_deps.size(); i++)
-    data_output_files.push_back(data_deps[i].ptr->dependency_output_file());
+  for (const auto& pair : data_deps)
+    data_output_files.push_back(pair.ptr->dependency_output_file());
 
   WriteStampForTarget(output_files, data_output_files);
 }
diff --git a/tools/gn/target.cc b/tools/gn/target.cc
index 9368749..def117f 100644
--- a/tools/gn/target.cc
+++ b/tools/gn/target.cc
@@ -30,11 +30,9 @@ void MergePublicConfigsFrom(const Target* from_target,
 void MergeAllDependentConfigsFrom(const Target* from_target,
                                   UniqueVector<LabelConfigPair>* dest,
                                   UniqueVector<LabelConfigPair>* all_dest) {
-  const UniqueVector<LabelConfigPair>& all =
-      from_target->all_dependent_configs();
-  for (size_t i = 0; i < all.size(); i++) {
-    all_dest->push_back(all[i]);
-    dest->push_back(all[i]);
+  for (const auto& pair : from_target->all_dependent_configs()) {
+    all_dest->push_back(pair);
+    dest->push_back(pair);
   }
 }
 
@@ -154,6 +152,16 @@ bool Target::IsFinal() const {
          (output_type_ == STATIC_LIBRARY && complete_static_lib_);
 }
 
+DepsIteratorRange Target::GetDeps(DepsIterationType type) const {
+  if (type == DEPS_LINKED) {
+    return DepsIteratorRange(DepsIterator(
+        &public_deps_, &private_deps_, nullptr));
+  }
+  // All deps.
+  return DepsIteratorRange(DepsIterator(
+      &public_deps_, &private_deps_, &data_deps_));
+}
+
 std::string Target::GetComputedOutputName(bool include_prefix) const {
   DCHECK(toolchain_)
       << "Toolchain must be specified before getting the computed output name.";
@@ -203,9 +211,8 @@ bool Target::SetToolchain(const Toolchain* toolchain, Err* err) {
 
 void Target::PullDependentTargetInfo() {
   // Gather info from our dependents we need.
-  for (DepsIterator iter(this, DepsIterator::LINKED_ONLY); !iter.done();
-       iter.Advance()) {
-    const Target* dep = iter.target();
+  for (const auto& pair : GetDeps(DEPS_LINKED)) {
+    const Target* dep = pair.ptr;
     MergeAllDependentConfigsFrom(dep, &configs_, &all_dependent_configs_);
     MergePublicConfigsFrom(dep, &configs_);
 
@@ -230,12 +237,12 @@ void Target::PullDependentTargetInfo() {
 
 void Target::PullForwardedDependentConfigs() {
   // Pull public configs from each of our dependency's public deps.
-  for (size_t dep = 0; dep < public_deps_.size(); dep++)
-    PullForwardedDependentConfigsFrom(public_deps_[dep].ptr);
+  for (const auto& dep : public_deps_)
+    PullForwardedDependentConfigsFrom(dep.ptr);
 
   // Forward public configs if explicitly requested.
-  for (size_t dep = 0; dep < forward_dependent_configs_.size(); dep++) {
-    const Target* from_target = forward_dependent_configs_[dep].ptr;
+  for (const auto& dep : forward_dependent_configs_) {
+    const Target* from_target = dep.ptr;
 
     // The forward_dependent_configs_ must be in the deps (public or private)
     // already, so we don't need to bother copying to our configs, only
@@ -257,18 +264,17 @@ void Target::PullForwardedDependentConfigsFrom(const Target* from) {
 }
 
 void Target::PullRecursiveHardDeps() {
-  for (DepsIterator iter(this, DepsIterator::LINKED_ONLY); !iter.done();
-       iter.Advance()) {
-    if (iter.target()->hard_dep())
-      recursive_hard_deps_.insert(iter.target());
+  for (const auto& pair : GetDeps(DEPS_LINKED)) {
+    if (pair.ptr->hard_dep())
+      recursive_hard_deps_.insert(pair.ptr);
 
     // Android STL doesn't like insert(begin, end) so do it manually.
     // TODO(brettw) this can be changed to
     // insert(iter.target()->begin(), iter.target()->end())
     // when Android uses a better STL.
     for (std::set<const Target*>::const_iterator cur =
-             iter.target()->recursive_hard_deps().begin();
-         cur != iter.target()->recursive_hard_deps().end(); ++cur)
+             pair.ptr->recursive_hard_deps().begin();
+         cur != pair.ptr->recursive_hard_deps().end(); ++cur)
       recursive_hard_deps_.insert(*cur);
   }
 }
@@ -333,8 +339,8 @@ void Target::FillOutputFiles() {
 }
 
 bool Target::CheckVisibility(Err* err) const {
-  for (DepsIterator iter(this); !iter.done(); iter.Advance()) {
-    if (!Visibility::CheckItemVisibility(this, iter.target(), err))
+  for (const auto& pair : GetDeps(DEPS_ALL)) {
+    if (!Visibility::CheckItemVisibility(this, pair.ptr, err))
       return false;
   }
   return true;
@@ -347,9 +353,9 @@ bool Target::CheckTestonly(Err* err) const {
     return true;
 
   // Verify no deps have "testonly" set.
-  for (DepsIterator iter(this); !iter.done(); iter.Advance()) {
-    if (iter.target()->testonly()) {
-      *err = MakeTestOnlyError(this, iter.target());
+  for (const auto& pair : GetDeps(DEPS_ALL)) {
+    if (pair.ptr->testonly()) {
+      *err = MakeTestOnlyError(this, pair.ptr);
       return false;
     }
   }
@@ -364,17 +370,17 @@ bool Target::CheckNoNestedStaticLibs(Err* err) const {
     return true;
 
   // Verify no deps are static libraries.
-  for (DepsIterator iter(this); !iter.done(); iter.Advance()) {
-    if (iter.target()->output_type() == Target::STATIC_LIBRARY) {
-      *err = MakeStaticLibDepsError(this, iter.target());
+  for (const auto& pair : GetDeps(DEPS_ALL)) {
+    if (pair.ptr->output_type() == Target::STATIC_LIBRARY) {
+      *err = MakeStaticLibDepsError(this, pair.ptr);
       return false;
     }
   }
 
   // Verify no inherited libraries are static libraries.
-  for (size_t i = 0; i < inherited_libraries().size(); ++i) {
-    if (inherited_libraries()[i]->output_type() == Target::STATIC_LIBRARY) {
-      *err = MakeStaticLibDepsError(this, inherited_libraries()[i]);
+  for (const auto& lib : inherited_libraries()) {
+    if (lib->output_type() == Target::STATIC_LIBRARY) {
+      *err = MakeStaticLibDepsError(this, lib);
       return false;
     }
   }
diff --git a/tools/gn/target.h b/tools/gn/target.h
index 9d4b4be..947edc2 100644
--- a/tools/gn/target.h
+++ b/tools/gn/target.h
@@ -23,6 +23,7 @@
 #include "tools/gn/source_file.h"
 #include "tools/gn/unique_vector.h"
 
+class DepsIteratorRange;
 class InputFile;
 class Settings;
 class Token;
@@ -41,6 +42,12 @@ class Target : public Item {
     ACTION,
     ACTION_FOREACH,
   };
+
+  enum DepsIterationType {
+    DEPS_ALL,  // Iterates through all public, private, and data deps.
+    DEPS_LINKED,  // Iterates through all non-data dependencies.
+  };
+
   typedef std::vector<SourceFile> FileList;
   typedef std::vector<std::string> StringVector;
 
@@ -125,6 +132,11 @@ class Target : public Item {
            output_type_ == COPY_FILES;
   }
 
+  // Returns the iterator range which can be used in range-based for loops
+  // to iterate over multiple types of deps in one loop:
+  //   for (const auto& pair : target->GetDeps(Target::DEPS_ALL)) ...
+  DepsIteratorRange GetDeps(DepsIterationType type) const;
+
   // Linked private dependencies.
   const LabelTargetVector& private_deps() const { return private_deps_; }
   LabelTargetVector& private_deps() { return private_deps_; }