This patch adds helpful comments.
http://codereview.appspot.com/5862052/diff/1/flower/include/direction.hh File flower/include/direction.hh (right): http://codereview.appspot.com/5862052/diff/1/flower/include/direction.hh#newcode77 flower/include/direction.hh:77: * Thanks to a #define below, instead of writing:
Only you and Han Wenn have answered. Maybe other agree on changing do to for_UP_and_DOWN? How do I know?
You cannot know every individual opinion, so you must decide what is wise. You know that I fear that I will forget if there is any difference between do{}while(flip()) and for_UP_and_DOWN. You know that HanWenn suggests changing all do{}while(flip) at once. If you change just a few do{}while(flip) to your new idiom, then I will be tempted to create a third idiom: for (Direction d = UP; d >= DOWN; d = (Direction)(d + DOWN - UP) http://codereview.appspot.com/5862052/diff/1/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5862052/diff/1/lily/note-collision.cc#newcode183 lily/note-collision.cc:183: */ Move the block comment above down below your new comments, to keep it adjacent to the if...elseif... block that it describes. http://codereview.appspot.com/5862052/diff/1/lily/note-collision.cc#newcode286 lily/note-collision.cc:286: // The offset should depend on line thickness, not staff space, at least in some cases (like stem-to-stem, where it should be bigger for smaller font size) What does "the offset" refer to? shift_amount? http://codereview.appspot.com/5862052/diff/1/lily/note-collision.cc#newcode383 lily/note-collision.cc:383: void update_offsets (Drul_array<vector<Real>
*offsets, What I was taught on the university is to write short and simple functions that do only one thing.
In this case, the function is too short, relative to the complexity of the interface(*), for separation. (*) The function changes what is pointed to by its first argument, which is not unusual, but milimetre88 found the same operation in another function worth a comment with a '!'. The second argument is used only for its dimensions, which are related to the dimensions of the first argument for reasons we can understand only by looking at the calling function. I would be confused wondering why the function does not simply use the dimensions of offsets[][]. http://codereview.appspot.com/5862052/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel