[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Rewrite Skyline code (issue 547980044 by address@hidden)
From: |
jonas . hahnfeld |
Subject: |
Re: Rewrite Skyline code (issue 547980044 by address@hidden) |
Date: |
Mon, 27 Apr 2020 00:01:27 -0700 |
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/
- Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/19
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), dak, 2020/04/19
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), nine . fierce . ballads, 2020/04/21
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/21
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/21
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), jonas . hahnfeld, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), dak, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden),
jonas . hahnfeld <=
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/27
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), jonas . hahnfeld, 2020/04/28
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/28
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), jonas . hahnfeld, 2020/04/28