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#newcode211 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#newcode67 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#newcode90 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#newcode116 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#newcode230 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#newcode284 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.hh#newcode31 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#newcode191 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#newcode373 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#newcode528 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#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:
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#newcode551 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-collision.cc;h=0354950a535f3856234e47a4e7904b2800cc9c09#l437> Done. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcode587 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.cc#newcode135 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. http://codereview.appspot.com/5651069/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel