Re: some comments and complaints on the code (issue 5651069)

[milimetr88]

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/