I generally agree with David that having the code agnostic of the container would be an improvement. This doesn't need to be a typedef yet as reserve() is unique to std::vector (at least not in std::list) and makes sense in the places it is used here. A rather obvious change (that I'm very surprised you didn't do in the first place) is replacing all iterator types by auto. Neither the code nor the reader should care what the type is, and it is long enough to considerably clutter the code. Even better, just use range-based loops, that should be standard C++ style by now. Also the patch is currently very hard to read because it still has the Interval changes that should be in master already (so it probably doesn't even apply).
What I'm not happy about is changing the patch status based on saying it's a "philosphical discussion". Every comment should be considered important, and it has happened often enough that something induces the submitter's creativity that might sound philosophical at first. https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode183 lily/skyline.cc:183: vector<Building>::iterator i; move into for loop and make auto https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode189 lily/skyline.cc:189: vector<Building>::iterator last = i; auto https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode216 lily/skyline.cc:216: vector<Building>::const_iterator cit = scp->begin (); auto (2x) https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode319 lily/skyline.cc:319: for (vector<Building>::const_iterator i = buildings.begin (); auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode565 lily/skyline.cc:565: vector<Building>::iterator end = buildings_.end (); auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode574 lily/skyline.cc:574: for (vector<Building>::iterator i = buildings_.begin (); i != end; i++) auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode620 lily/skyline.cc:620: for (vector<Building>::const_iterator i = buildings_.begin (); auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode677 lily/skyline.cc:677: vector<Building>::const_iterator j = other.buildings_.begin (); auto (2x) https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode711 lily/skyline.cc:711: vector<Building>::const_iterator i; move into for and auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode727 lily/skyline.cc:727: vector<Building>::const_iterator i; move into for and auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode748 lily/skyline.cc:748: for (vector<Building>::const_iterator i (buildings_.begin ()); auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode760 lily/skyline.cc:760: for (vector<Building>::const_reverse_iterator i (buildings_.rbegin ()); auto or range-based for https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode792 lily/skyline.cc:792: for (vector<Building>::const_iterator i (buildings_.begin ()); auto or range-based for https://codereview.appspot.com/583750043/
