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

Issue 5975054: Corrected comments and a function check_meshing_chords divided in two (issue 2310)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 3 months ago by Milimetr88
Modified:
7 years, 2 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Corrected comments and a function check_meshing_chords divided in two. http://code.google.com/p/lilypond/issues/detail?id=2310

Patch Set 1 #

Total comments: 27

Patch Set 2 : Removed for_UP_and_DOWN and corrected the code according to comments. #

Total comments: 2

Patch Set 3 : Some corrections of comments' style. Could we finish with this patch? #

Unified diffs Side-by-side diffs Delta from patch set Stats (+193 lines, -85 lines) Patch
M flower/include/direction.hh View 1 1 chunk +3 lines, -1 line 0 comments Download
M lily/accidental-placement.cc View 1 2 6 chunks +15 lines, -7 lines 0 comments Download
M lily/grob.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M lily/include/grob-interface.hh View 1 chunk +1 line, -1 line 0 comments Download
M lily/include/note-collision.hh View 1 1 chunk +25 lines, -0 lines 0 comments Download
M lily/note-collision.cc View 1 2 15 chunks +133 lines, -73 lines 0 comments Download
M lily/note-column.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M lily/staff-symbol-referencer.cc View 1 2 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 18
Keith
LGTM, except that it confuses the two programs we have used recently for automatic code ...
7 years, 3 months ago (2012-03-31 21:04:35 UTC) #1
Graham Percival
could you split this into 2 (or more) separate patches? - some of your comments ...
7 years, 3 months ago (2012-04-01 05:00:24 UTC) #2
Keith
On 2012/04/01 05:00:25, Graham Percival wrote: > Ouch. I don't find that for loop to ...
7 years, 3 months ago (2012-04-01 06:12:26 UTC) #3
Graham Percival
On Sun, Apr 01, 2012 at 06:12:27AM +0000, k-ohara5a5a@oco.net wrote: > On 2012/04/01 05:00:25, Graham ...
7 years, 3 months ago (2012-04-01 06:26:04 UTC) #4
hanwenn
http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh File lily/include/note-collision.hh (right): http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh#newcode56 lily/include/note-collision.hh:56: int down_ball_type; this doesnt make sense? The booleans classify ...
7 years, 3 months ago (2012-04-02 01:03:40 UTC) #5
Milimetr88
These corrections are included in the Patch Set 2. http://codereview.appspot.com/5975054/diff/1/flower/include/direction.hh File flower/include/direction.hh (right): http://codereview.appspot.com/5975054/diff/1/flower/include/direction.hh#newcode90 flower/include/direction.hh:90: ...
7 years, 3 months ago (2012-04-14 13:56:18 UTC) #6
Graham Percival
On 2012/04/14 13:56:18, Milimetr88 wrote: > On 2012/03/31 21:04:35, Keith wrote: > > It is ...
7 years, 3 months ago (2012-04-14 14:18:58 UTC) #7
Milimetr88
http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc#newcode116 lily/accidental-placement.cc:116: //TODO: add comment to this struct I'll delete that.
7 years, 3 months ago (2012-04-14 14:31:24 UTC) #8
Milimetr88
http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode584 lily/note-collision.cc:584: || (extents[-d].size () && d * (extents[d][i][-d] - extents[-d][0][d]) ...
7 years, 3 months ago (2012-04-14 14:36:23 UTC) #9
Milimetr88
On 14 April 2012 16:18, <graham@percival-music.ca> wrote: > > > I have no >> experience ...
7 years, 3 months ago (2012-04-14 14:39:36 UTC) #10
dak
Without wanting to rain on your parade: what is all this about? I actually had ...
7 years, 3 months ago (2012-04-14 14:52:33 UTC) #11
Milimetr88
On 14 April 2012 16:52, <dak@gnu.org> wrote: > Without wanting to rain on your parade: ...
7 years, 3 months ago (2012-04-14 15:21:26 UTC) #12
dak
On 2012/04/14 15:21:26, Milimetr88 wrote: > > > I didn't expected so long reply. The ...
7 years, 3 months ago (2012-04-14 17:01:36 UTC) #13
Keith
Splitting the function in two doesn't make it any easier for me to understand, but ...
7 years, 3 months ago (2012-04-14 19:12:01 UTC) #14
dak
On 2012/04/14 19:12:01, Keith wrote: > Splitting the function in two doesn't make it any ...
7 years, 3 months ago (2012-04-14 20:37:06 UTC) #15
Milimetr88
http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc File lily/staff-symbol-referencer.cc (right): http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#newcode126 lily/staff-symbol-referencer.cc:126: * Returns vertical position of a symbol relatively to ...
7 years, 3 months ago (2012-04-15 13:52:36 UTC) #16
Milimetr88
On 14 April 2012 21:12, <k-ohara5a5a@oco.net> wrote: > Splitting the function in two doesn't make ...
7 years, 3 months ago (2012-04-15 13:58:16 UTC) #17
Milimetr88
7 years, 2 months ago (2012-04-24 17:36:27 UTC) #18
http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc#...
lily/accidental-placement.cc:116: //TODO: add comment to this struct
On 2012/04/14 14:31:24, Milimetr88 wrote:
> I'll delete that.

Done.
Sign in to reply to this message.

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