Re: Use Interval for representing skyline X coordinates (issue 571980043 by address@hidden)

[hanwenn]

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/