Re: Use vectors rather than lists for skylines. (issue 583750043 by address@hidden)

[jonas . hahnfeld]

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/