Re: Rewrite Skyline code (issue 547980044 by address@hidden)

[jonas . hahnfeld]

https://codereview.appspot.com/547980044/diff/572060043/lily/include/skyline.hh
File lily/include/skyline.hh (right):

https://codereview.appspot.com/547980044/diff/572060043/lily/include/skyline.hh#newcode34
lily/include/skyline.hh:34: #include "offset.hh"
Please don't separate the lily includes - actually interval.hh is
already included above

https://codereview.appspot.com/547980044/diff/572060043/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/547980044/diff/572060043/lily/skyline.cc#newcode28
lily/skyline.cc:28: #include "skyline.hh"
As discussed in other reviews, skyline.hh should be the very first
include and system headers should be last

https://codereview.appspot.com/547980044/diff/572060043/lily/skyline.cc#newcode171
lily/skyline.cc:171: class BuildingQueue
Do you really need a custom type for this? It really much looks like an
iterator (and you can call methods on the current item by using the ->
operator)

https://codereview.appspot.com/547980044/diff/572060043/lily/skyline.cc#newcode175
lily/skyline.cc:175: Building front_;
In any case, this causes copies on every call to next(). If you need to
retain this data structure, is there a reason not to use a pointer to
the current element? AFAICS you don't modify the underlying vector when
using a BuildingQueue

https://codereview.appspot.com/547980044/diff/572060043/lily/skyline.cc#newcode365
lily/skyline.cc:365: class LessThanBuilding
Can we just replace this functor by passing Building::less as a function
pointer?

https://codereview.appspot.com/547980044/