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#newcode39 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-referencer.hh File lily/include/staff-symbol-referencer.hh (right): http://codereview.appspot.com/5651069/diff/1/lily/include/staff-symbol-referencer.hh#newcode53 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. http://codereview.appspot.com/5651069/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel