Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(566)

Issue 571980043: Use Interval for representing skyline X coordinates (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 11 months ago by hanwenn
Modified:
3 years, 11 months ago
Reviewers:
dak, hahnjo, Dan Eble
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Use Interval for representing skyline X coordinates

Patch Set 1 #

Total comments: 6

Patch Set 2 : dak/hahnjo #

Patch Set 3 : dan #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -77 lines) Patch
M lily/include/skyline.hh View 1 2 1 chunk +2 lines, -3 lines 0 comments Download
M lily/skyline.cc View 1 2 20 chunks +71 lines, -74 lines 4 comments Download

Messages

Total messages: 10
hahnjo
Is there a reason Interval is better? Please add a description https://codereview.appspot.com/571980043/diff/579590043/lily/include/skyline.hh File lily/include/skyline.hh (right): ...
3 years, 11 months ago (2020-04-16 20:14:50 UTC) #1
dak
Purpose is not clear since the code does not appear to use anything other than ...
3 years, 11 months ago (2020-04-16 20:27:55 UTC) #2
hanwenn
dak/hahnjo
3 years, 11 months ago (2020-04-16 20:45:16 UTC) #3
hanwenn
This a cleanup. Most of lily tries to use Offset/Interval rather than enumerating the Real ...
3 years, 11 months ago (2020-04-16 20:46:54 UTC) #4
Dan Eble
The idea makes sense to me. I haven't reviewed the whole change. https://codereview.appspot.com/571980043/diff/579590043/lily/skyline.cc File lily/skyline.cc ...
3 years, 11 months ago (2020-04-16 20:50:20 UTC) #5
hanwenn
dan
3 years, 11 months ago (2020-04-16 20:57:01 UTC) #6
hanwenn
https://codereview.appspot.com/571980043/diff/579590043/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/571980043/diff/579590043/lily/skyline.cc#newcode100 lily/skyline.cc:100: x_[START] = start; On 2020/04/16 20:50:20, Dan Eble wrote: ...
3 years, 11 months ago (2020-04-16 20:57:16 UTC) #7
Dan Eble
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] ...
3 years, 11 months ago (2020-04-16 21:44:00 UTC) #8
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): ...
3 years, 11 months ago (2020-04-17 11:23:38 UTC) #9
hanwenn
3 years, 11 months ago (2020-05-02 22:22:57 UTC) #10
commit c5b720596fc967baac29cfc4b674b76927377135
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Thu Apr 16 22:03:26 2020 +0200

    Use Interval for representing skyline X coordinates
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b