Issue 5651069: some comments and complaints on the code (issue 2310) (Closed)
|
Created:
12 years, 2 months ago by janek Modified:
12 years ago CC:
lilypond-devel_gnu.org Base URL:
http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/ Visibility:
Public. |
Descriptionsome comments and complaints on the code
direction.hh: added a #define loop for_UP_and_DOWN()
for iterating through: {UP, DOWN}
minor changes in note-collision.cc, some comments etc.
Patch Set 1 #
Total comments: 30
Patch Set 2 : address style comments #
Total comments: 29
MessagesTotal messages: 16
Hi, together with Luke (milimetr88@gmail.com) we're preparing a fix for http://code.google.com/p/lilypond/issues/detail?id=1546 We had a really hard time on Wednesday trying to understand how the current code works; after 4 hours of reading we have some grasp now. In order not to loose this knowledge, we decided to add it as comments - it'd be good if you confirmed we understood things correctly. We also added some questions - if you can answer them, it would be great!
Sign in to reply to this message.
Thanks for taking this on, Janek.
I don't know what the response will be to for_UP_and_DOWN(d). The last time
somebody proposed a change, it was resisted because the do{} flip(d)!=UP idiom
seemed simple enough to be acceptable.
But I think the new idiom is more readable, and if there are no performance
issues with it, I'd be in favor of it.
I have some specific comments below about spacing, comment format, and comment
placement. In general, I think it's a good idea for you to add comments. I'm
not sure it's a good idea to add TODO about missing comments (but it might be).
I'm really hesitant to have you add comments that say "please add a comment",
since it's not clear to whom the please is directed.
Thanks,
Carl
http://codereview.appspot.com/5651069/diff/1/lily/include/note-column.hh
File lily/include/note-column.hh (right):
http://codereview.appspot.com/5651069/diff/1/lily/include/note-column.hh#newc...
lily/include/note-column.hh:39: /**
IMO, this comment belongs in the definition, not the declaration
http://codereview.appspot.com/5651069/diff/1/lily/include/staff-symbol-refere...
File lily/include/staff-symbol-referencer.hh (right):
http://codereview.appspot.com/5651069/diff/1/lily/include/staff-symbol-refere...
lily/include/staff-symbol-referencer.hh:53: /**
Again, comment in the definition, not the declaration
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode369
lily/note-collision.cc:369: ->set_property ("direction", scm_from_int (dir));
I think the indentation in the old code is correct. The -> should be indented,
since it's not the start of a statement.
But if this spacing is what's produced by fixcc.py, then we should keep it.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode377
lily/note-collision.cc:377: /**
I don't like the double * following the /. Is this a standard anywhere else in
lilypond?
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode552
lily/note-collision.cc:552: for_UP_and_DOWN (d) // please, make a comment to
this loop (better than the above one...)
To whom is the "please" directed? Is there anybody who is better than you right
now to comment the loop?
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode588
lily/note-collision.cc:588: {
I don't know that we have a specification for this, or if it can be handled by
fixcc.py, but for consistency with the way we indent braces with loops and if,
the braces should be indented two spaces, IMO.
Sign in to reply to this message.
http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#new... lily/accidental-placement.cc:211: * @return A vector of Accidental_placement_entrys Please don't just comment on the type (unless it's SCM in which case you can say which scheme type it is).
Sign in to reply to this message.
On 2012/02/10 23:49:24, Carl wrote:
> Thanks for taking this on, Janek.
>
> I don't know what the response will be to for_UP_and_DOWN(d). The last time
> somebody proposed a change, it was resisted because the do{} flip(d)!=UP idiom
> seemed simple enough to be acceptable.
It took us a while to figure out what's going on with the do{} flip(d)!=UP
thing.
If it was up to me, i'd just write everything twice, following the rule "1, 2,
many" :)
Some of your comments will be better addressed by Luke - i leave them to him.
Thanks for review!
Janek
http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):
http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#new...
lily/accidental-placement.cc:211: * @return A vector of
Accidental_placement_entrys
On 2012/02/11 00:09:00, joeneeman wrote:
> Please don't just comment on the type (unless it's SCM in which case you can
say
> which scheme type it is).
Well, the main point of the comment is not describing parameters, but the
function itself. Believe me or not, we spent 10 minutes figuring out _what the
hell_ are apes doing here and whether there are any bananas involved.
It's stupid, i know. But there must exist a way of writing code that is
understandable for the newcomers after second reading, not after 10 minutes.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode316
lily/note-collision.cc:316: shift_amount *= 1.25;
this must be a mistake actually. We didn't intend to do anything with the
working code yet.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode369
lily/note-collision.cc:369: ->set_property ("direction", scm_from_int (dir));
On 2012/02/10 23:49:24, Carl wrote:
> I think the indentation in the old code is correct. The -> should be
indented,
> since it's not the start of a statement.
+1
> But if this spacing is what's produced by fixcc.py, then we should keep it.
It was done by fixcc indeed.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode552
lily/note-collision.cc:552: for_UP_and_DOWN (d) // please, make a comment to
this loop (better than the above one...)
On 2012/02/10 23:49:24, Carl wrote:
> To whom is the "please" directed? Is there anybody who is better than you
right
> now to comment the loop?
Yes: the author of the code. There are also other people in our team who will
understand this at second reading; i still don't understand how exactly this
works.
(of course i don't demand that anyone does anything about it)
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode588
lily/note-collision.cc:588: {
On 2012/02/10 23:49:24, Carl wrote:
> I don't know that we have a specification for this, or if it can be handled by
> fixcc.py, but for consistency with the way we indent braces with loops and if,
> the braces should be indented two spaces, IMO.
+10
However, the result was produced with fixcc. At the moment i don't know how to
fix fixcc; i've looked at its code and it contains a lot of
('([\w\)\]]) *(--|\+\+)', '\\1\\2')
which look nice but i have absolutely no idea how they work, and no time to
learn at the moment :(
Sign in to reply to this message.
Forgot about one thing, sorry http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode552 lily/note-collision.cc:552: for_UP_and_DOWN (d) // please, make a comment to this loop (better than the above one...) I've just realized that the correct answer for your question is On 2012/02/10 23:49:24, Carl wrote: > To whom is the "please" directed? Is there anybody who is > better than you right now to comment the loop? Me in the future :) When we finish this bugfix, we'll hopefully understand everything well, and this comment will remind us to write down this knowledge :D
Sign in to reply to this message.
http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#new... lily/accidental-placement.cc:211: * @return A vector of Accidental_placement_entrys Do you mean @param and @return or the comment to the function? What comment would you propose? On 2012/02/11 00:09:00, joeneeman wrote: > Please don't just comment on the type (unless it's SCM in which case you can say > which scheme type it is). http://codereview.appspot.com/5651069/diff/1/lily/include/note-column.hh File lily/include/note-column.hh (right): http://codereview.appspot.com/5651069/diff/1/lily/include/note-column.hh#newc... lily/include/note-column.hh:39: /** Ok, I'll correct that. On 2012/02/10 23:49:24, Carl wrote: > IMO, this comment belongs in the definition, not the declaration http://codereview.appspot.com/5651069/diff/1/lily/include/note-column.hh#newc... lily/include/note-column.hh:39: /** On 2012/02/10 23:49:24, Carl wrote: > IMO, this comment belongs in the definition, not the declaration Done. http://codereview.appspot.com/5651069/diff/1/lily/include/staff-symbol-refere... File lily/include/staff-symbol-referencer.hh (right): http://codereview.appspot.com/5651069/diff/1/lily/include/staff-symbol-refere... lily/include/staff-symbol-referencer.hh:53: /** Ok, I'll correct that. On 2012/02/10 23:49:24, Carl wrote: > Again, comment in the definition, not the declaration http://codereview.appspot.com/5651069/diff/1/lily/include/staff-symbol-refere... lily/include/staff-symbol-referencer.hh:53: /** On 2012/02/10 23:49:24, Carl wrote: > Again, comment in the definition, not the declaration Done. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode377 lily/note-collision.cc:377: /** AFAIK, not really. In Contributor's Guide there are only 2 short paragraphs: Comments may not be needed if descriptive variable names are used in the code and the logic is straightforward. However, if the logic is difficult to follow, and particularly if non-obvious code has been included to resolve a bug, a comment describing the logic and/or the need for the non-obvious code should be included. There are instances where the current code could be commented better. If significant time is required to understand the code as part of preparing a patch, it would be wise to add comments reflecting your understanding to make future work easier. (http://lilypond.org/doc/v2.15/Documentation/contributor-big-page#code-comments) I just wanted to introduce JavaDoc comment style for autodocumenting. Maybe style of comments should be discussed on lilypond-devel? On 2012/02/10 23:49:24, Carl wrote: > I don't like the double * following the /. Is this a standard anywhere else in > lilypond? http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode552 lily/note-collision.cc:552: for_UP_and_DOWN (d) // please, make a comment to this loop (better than the above one...) On 2012/02/10 23:49:24, Carl wrote: > To whom is the "please" directed? Is there anybody who is better than you right > now to comment the loop? Yes - someone who knows this code :] Possibly one day someone will try to refactor this code, what will mean that he understands it well. For me it's really hard to understand whats going on here. Me and Janek recently spent a couple of hours trying to understand several files including this one and we still don't know if clash_groups[d] is one single note or a group of notes. Do you have a tip, how to work it out? Unfortunately it's not state in the code.
Sign in to reply to this message.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode588 lily/note-collision.cc:588: { On 2012/02/10 23:49:24, Carl wrote: > I don't know that we have a specification for this, or if it can be handled by > fixcc.py, but for consistency with the way we indent braces with loops and if, > the braces should be indented two spaces, IMO. Done. Janek will push it soon. Maybe fixcc will not change it back...
Sign in to reply to this message.
new patch set uploaded, please review. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode588 lily/note-collision.cc:588: { On 2012/02/11 12:32:57, Milimetr88 wrote: > On 2012/02/10 23:49:24, Carl wrote: > > I don't know that we have a specification for this, or if it can be handled by > > fixcc.py, but for consistency with the way we indent braces with loops and if, > > the braces should be indented two spaces, IMO. > > Done. Janek will push it soon. Maybe fixcc will not change it back... I've checked that fixcc will change this if it's ran on the file. No need to do this, though. I'll add an issue about fixing fixcc.
Sign in to reply to this message.
http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:191: */ Protect the comment formatting with a column of '*'s Otherwise, someone might let emacs re-align the comment, as happened at line 543. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:536: s[LEFT]--; // why LEFT and RIGHT instead of UP and DOWN? These lines first appeared in version 1.1.49, thirteen years ago. If you do not get answers to your questions during this review, you will probably have to find the answers yourself. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:551: You can use `git blame` to find the original form of this comment <http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=blob;f=lily/note-collisi...> http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:587: for_UP_and_DOWN (d) Now we have both for_UP_and_DOWN and the while(flip()) idioms in LilyPond, so every time I forget what the difference is, I have to search for the definition, open a different file, and study an even more complicated loop construction. If you think that for_UP_or_DOWN was self-documenting, then better to : do // for d = UP, then DOWN { ...
Sign in to reply to this message.
http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#new... lily/accidental-placement.cc:211: * @return A vector of Accidental_placement_entrys On 2012/02/11 12:27:31, Milimetr88 wrote: > Do you mean @param and @return or the comment to the function? What comment > would you propose? I'm complaining about the @return comment, since it's redundant (ie. no need to change it, just remove that line). I'm ok with the @param comment, because you can't tell from the function signature that accs is supposed to be a list.
Sign in to reply to this message.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode536 lily/note-collision.cc:536: s[LEFT]--; // why LEFT and RIGHT instead of UP and DOWN? sometimes i liked to think about intervals as lying on the horizontal line. Feel free to change to up/down. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode539 lily/note-collision.cc:539: offsets[d].push_back (d * 0.5 * i); // what for? you can run a regtest after removing this to be sure, but afaics, this just takes care that the hshift = 2 group is shifted relative to the hshift = 1 group, and similar for 3, 4, 5 .. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode559 lily/note-collision.cc:559: || (extents[-d].size () && d * (extents[d][i][-d] - extents[-d][0][d]) < 0)) // please, comment this condition expanding for d = UP extents[UP][i][DOWN] < extents[DOWN][0][UP] .. offsets[UP][..all..] += 0.5 ie. if the upper reaches into area of the lower groups, then shift the upper to the right. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode587 lily/note-collision.cc:587: for_UP_and_DOWN (d) after reading some of the code, this actually looks nice. Can you rename it for_updown ? for brevity and writability? There should also be a for_leftright() http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh File flower/include/direction.hh (right): http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh#n... flower/include/direction.hh:67: */ If you think there should be doxygen style commenting, rather than doing these one off, build consensus on the list, and fix all comments in the codebase in one go. I personally think they look terribly verbose, and are unpleasant to read in the sourcecode. Concise english sentences work better than @awkward comments to document things, IMO. http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh#n... flower/include/direction.hh:90: */ if we decide to go ahead with this, there is no need to refer to the old style syntax, nor is it necessary to call the old one ugly. http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#... lily/accidental-placement.cc:116: //TODO: add comment to this class drop http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#... lily/accidental-placement.cc:230: //TODO: please add comment to this function this sets the skylines on the ape argument. http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#... lily/accidental-placement.cc:284: //TODO: please add comment to this function this function does exactly what its name says it does: it extract the noteheads and stems from its argument. http://codereview.appspot.com/5651069/diff/5002/lily/include/grob-interface.hh File lily/include/grob-interface.hh (right): http://codereview.appspot.com/5651069/diff/5002/lily/include/grob-interface.h... lily/include/grob-interface.hh:31: bool cl::has_interface (Grob *me) /* TODO: please add comment to this function */ \ either do it or leave, otherwise we could adorn the entire codebase with TODO. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:373: update_offsets (offsets, clash_groups, shift_amount); there is not much value in abstracting into a function unless you can use the function at least twice. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:528: Drul_array<vector<Slice> > extents; // vertical extends extents, not extends http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:536: s[LEFT]--; // why LEFT and RIGHT instead of UP and DOWN? On 2012/02/12 01:29:14, Keith wrote: > These lines first appeared in version 1.1.49, thirteen years ago. If you do not > get answers to your questions during this review, you will probably have to find > the answers yourself. the reason is that 535 gets extents in posititions, but a single note head occupies 3 positions, since the symbol has thickness, hence the -- and ++ http://codereview.appspot.com/5651069/diff/5002/lily/note-column.cc File lily/note-column.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/note-column.cc#newcode68 lily/note-column.cc:68: * Returns a Slice (an interval of half-staff spaces) which ends are the lowest and highest note head respectively this comment doesn't add any information that the name already conveys, IMO. Slice is not so much an interval of halfspaces, but it is an interval of integers. The assumption is that heads are always either on or between staff lines. http://codereview.appspot.com/5651069/diff/5002/lily/staff-symbol-referencer.cc File lily/staff-symbol-referencer.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/staff-symbol-referencer.... lily/staff-symbol-referencer.cc:135: */ again, this is all is implicit in the name. 'position' refers to the integer vertical position, 0 being the center of the staff.
Sign in to reply to this message.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode587 lily/note-collision.cc:587: for_UP_and_DOWN (d) On 2012/02/13 11:33:48, hanwenn wrote: > after reading some of the code, this actually looks nice. > > Can you rename it > > for_updown ? > > for brevity and writability? There should also be a for_leftright() also, this change should be separated out. Build consensus on the list, and then fix all of the codebase in one go.
Sign in to reply to this message.
On Sat, Feb 11, 2012 at 8:31 AM, <janek.lilypond@gmail.com> wrote: >> somebody proposed a change, it was resisted because the do{} > > flip(d)!=UP idiom >> >> seemed simple enough to be acceptable. > > > It took us a while to figure out what's going on with the do{} > flip(d)!=UP thing. > If it was up to me, i'd just write everything twice, following the rule > "1, 2, many" :) it would double the size of already rather complicated code. My experience has also been that its easy for a bug to sneak into the more complicated expressions. To make things worse, you typically have to double the number of test cases to provide coverage for both up and down versions of all logic. > Well, the main point of the comment is not describing parameters, but > the function itself. Believe me or not, we spent 10 minutes figuring > out _what the hell_ are apes doing here and whether there are any > bananas involved. > It's stupid, i know. But there must exist a way of writing code that is > understandable for the newcomers after second reading, not after 10 > minutes. I disagree. Reading code the first time is hard, that is true, but unless anything surprising is happening, it does not deserve a comment. The more you read code, the easier it becomes, and think of this as you learning how to read. In an analog: beginners books may feature simpler words, hy-phe-na-te all words explicitly and use pictures, but that doesn't mean literature for grownups should have those. There is also a practical reason to minimize: comments easily are forgotten when changing code, so they tend go out of date faster. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
On Sat, Feb 11, 2012 at 10:27 AM, <milimetr88@gmail.com> wrote: > Yes - someone who knows this code :] Possibly one day someone will try > to refactor this code, what will mean that he understands it well. > For me it's really hard to understand whats going on here. Me and Janek > recently spent a couple of hours trying to understand several files > including this one and we still don't know if clash_groups[d] is one > single note or a group of notes. > > Do you have a tip, how to work it out? Unfortunately it's not state in > the code. See the get_clash_groups function. You can figure this out for yourself by walking up the call-chain until you find the function that populates the variable. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
Dear Han-Wen, Keith and Joe, many thanks for your comments and advice! I appreciate your feedback and apologize that i didn't respond yet; i was waiting for Łukasz (milimetr88@gmail) to decide together what we shall do, but he's currently busy :/ Please give us a few more days. Meanwhile, i set its status to needs_work. thanks, Janek
Sign in to reply to this message.
The next patch set was by mistake placed by me in a new issue: http://codereview.appspot.com/5862052. http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#new... lily/accidental-placement.cc:211: * @return A vector of Accidental_placement_entrys On 2012/02/13 03:36:27, joeneeman wrote: > On 2012/02/11 12:27:31, Milimetr88 wrote: > > Do you mean @param and @return or the comment to the function? What comment > > would you propose? > > I'm complaining about the @return comment, since it's redundant (ie. no need to > change it, just remove that line). I'm ok with the @param comment, because you > can't tell from the function signature that accs is supposed to be a list. > Done. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode536 lily/note-collision.cc:536: s[LEFT]--; // why LEFT and RIGHT instead of UP and DOWN? On 2012/02/13 11:33:48, hanwenn wrote: > sometimes i liked to think about intervals as lying on the horizontal line. Feel > free to change to up/down. Done. http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh File flower/include/direction.hh (right): http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh#n... flower/include/direction.hh:67: */ On 2012/02/13 11:33:48, hanwenn wrote: > If you think there should be doxygen style commenting, rather than doing these > one off, build consensus on the list, and fix all comments in the codebase in > one go. > > I personally think they look terribly verbose, and are unpleasant to read in the > sourcecode. Concise english sentences work better than @awkward comments to > document things, IMO. Done. http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh#n... flower/include/direction.hh:90: */ On 2012/02/13 11:33:48, hanwenn wrote: > if we decide to go ahead with this, there is no need to refer to the old style > syntax, nor is it necessary to call the old one ugly. Done. http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#... lily/accidental-placement.cc:116: //TODO: add comment to this class On 2012/02/13 11:33:48, hanwenn wrote: > drop Done. http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#... lily/accidental-placement.cc:230: //TODO: please add comment to this function On 2012/02/13 11:33:48, hanwenn wrote: > this sets the skylines on the ape argument. Done. http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#... lily/accidental-placement.cc:284: //TODO: please add comment to this function On 2012/02/13 11:33:48, hanwenn wrote: > this function does exactly what its name says it does: it extract the noteheads > and stems from its argument. Done. http://codereview.appspot.com/5651069/diff/5002/lily/include/grob-interface.hh File lily/include/grob-interface.hh (right): http://codereview.appspot.com/5651069/diff/5002/lily/include/grob-interface.h... lily/include/grob-interface.hh:31: bool cl::has_interface (Grob *me) /* TODO: please add comment to this function */ \ On 2012/02/13 11:33:48, hanwenn wrote: > either do it or leave, otherwise we could adorn the entire codebase with TODO. Done. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:191: */ On 2012/02/12 01:29:14, Keith wrote: > Protect the comment formatting with a column of '*'s > Otherwise, someone might let emacs re-align the comment, as happened at line > 543. Done. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:373: update_offsets (offsets, clash_groups, shift_amount); On 2012/02/13 11:33:48, hanwenn wrote: > there is not much value in abstracting into a function unless you can use the > function at least twice. What I was taught on the university is to write short and simple functions that do only one thing. Kind of a measure can be a test if a function takes more than one screen. I know that this function is a small one, but it encloses a separate functionality. May I leave it or revert to previous state? http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:528: Drul_array<vector<Slice> > extents; // vertical extends On 2012/02/13 11:33:48, hanwenn wrote: > extents, not extends Done. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:536: s[LEFT]--; // why LEFT and RIGHT instead of UP and DOWN? On 2012/02/13 11:33:48, hanwenn wrote: > On 2012/02/12 01:29:14, Keith wrote: > > These lines first appeared in version 1.1.49, thirteen years ago. If you do > not > > get answers to your questions during this review, you will probably have to > find > > the answers yourself. > > the reason is that 535 gets extents in posititions, but a single note head > occupies 3 positions, since the symbol has thickness, hence the -- and ++ Well, that's not an answer on my question, but never mind, I'll insert that as a comment. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:551: On 2012/02/12 01:29:14, Keith wrote: > You can use `git blame` to find the original form of this comment > <http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=blob;f=lily/note-collisi...> Done. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod... lily/note-collision.cc:587: for_UP_and_DOWN (d) On 2012/02/12 01:29:14, Keith wrote: > Now we have both for_UP_and_DOWN and the while(flip()) idioms in LilyPond, so > every time I forget what the difference is, I have to search for the definition, > open a different file, and study an even more complicated loop construction. > > If you think that for_UP_or_DOWN was self-documenting, then better to : > do // for d = UP, then DOWN > { > ... Only you and Han Wenn have answered. Maybe other agree on changing do to for_UP_and_DOWN? How do I know? http://codereview.appspot.com/5651069/diff/5002/lily/note-column.cc File lily/note-column.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/note-column.cc#newcode68 lily/note-column.cc:68: * Returns a Slice (an interval of half-staff spaces) which ends are the lowest and highest note head respectively On 2012/02/13 11:33:48, hanwenn wrote: > this comment doesn't add any information that the name already conveys, IMO. > > Slice is not so much an interval of halfspaces, but it is an interval of > integers. The assumption is that heads are always either on or between staff > lines. The last sentence looks like a good one to be inserted in such a comment, don't you think? :) http://codereview.appspot.com/5651069/diff/5002/lily/staff-symbol-referencer.cc File lily/staff-symbol-referencer.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/staff-symbol-referencer.... lily/staff-symbol-referencer.cc:135: */ On 2012/02/13 11:33:48, hanwenn wrote: > again, this is all is implicit in the name. 'position' refers to the integer > vertical position, 0 being the center of the staff. Is there any place in documentation or code where it's written that 0 is the center of the staff? Some time ago I tried to find a comment about that, but didn't succeed.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
