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

Issue 554000043: Avoid using Skyline_pair::insert. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by hanwenn
Modified:
4 weeks, 1 day ago
Reviewers:
hahnjo
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Avoid using Skyline_pair::insert. Skyline::insert(Box) creates a skyline and merges it. In this case, the boxes are non-overlapping, so it is trivial to create a skyline out of them. Remove Skyline::insert(). Using it repeatedly leads to a quadratic complexity, so it's best avoided. Timing benchmarks are neutral

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -71 lines) Patch
M lily/align-interface.cc View 1 chunk +41 lines, -41 lines 2 comments Download
M lily/include/skyline-pair.hh View 1 chunk +0 lines, -1 line 0 comments Download
M lily/skyline.cc View 1 chunk +0 lines, -22 lines 0 comments Download
M lily/skyline-pair.cc View 2 chunks +2 lines, -7 lines 0 comments Download

Messages

Total messages: 2
hahnjo
https://codereview.appspot.com/554000043/diff/549990043/lily/align-interface.cc File lily/align-interface.cc (right): https://codereview.appspot.com/554000043/diff/549990043/lily/align-interface.cc#newcode90 lily/align-interface.cc:90: Real offset = g->relative_coordinate (other_common, other_axis (a)); Is this ...
1 month ago (2020-05-04 08:12:32 UTC) #1
hanwenn
1 month ago (2020-05-04 09:14:47 UTC) #2
https://codereview.appspot.com/554000043/diff/549990043/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/554000043/diff/549990043/lily/align-interface....
lily/align-interface.cc:90: Real offset = g->relative_coordinate (other_common,
other_axis (a));
On 2020/05/04 08:12:32, hahnjo wrote:
> Is this code intentionally moving into if (skys) ?

yes. Moving around empty skylines is a nop.
Sign in to reply to this message.

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