On 2020/04/16 21:44:00, Dan Eble wrote: > LGTM > > https://codereview.appspot.com/571980043/diff/581920043/lily/skyline.cc > File lily/skyline.cc (right): > > https://codereview.appspot.com/571980043/diff/581920043/lily/skyline.cc#newcode164 > lily/skyline.cc:164: if (ret >= x_[LEFT] && ret <= x_[RIGHT] && !std::isinf > (ret)) > Is this x_.contains (ret) && ...? > > https://codereview.appspot.com/571980043/diff/581920043/lily/skyline.cc#newcode223 > lily/skyline.cc:223: if (b.x_[RIGHT] < c.x_[RIGHT]) /* finish with b */ > Hmm... I wonder if there are Interval operations that would made this section > more readable. I'm not asking you to do anything, just wondering. > > https://codereview.appspot.com/571980043/diff/581920043/lily/skyline.cc#newcode292 > lily/skyline.cc:292: assert (b.x_[RIGHT] >= b.x_[LEFT]); > Is this !b.x_.is_empty ()? > > https://codereview.appspot.com/571980043/diff/581920043/lily/skyline.cc#newcode565 > lily/skyline.cc:565: i->x_[LEFT] += s; > Is this i->x_ += s?
you're right about many of those, but I'll leave it for a follow-up change. There might be subtleties in handling >= vs > . I also want to take a closer look at the skyline code; I think it may be allocating too much data https://codereview.appspot.com/571980043/
