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

[hanwenn]

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#newcode67
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#newcode90
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#newcode116
lily/accidental-placement.cc:116: //TODO: add comment to this class
drop

http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#newcode230
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#newcode284
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.hh#newcode31
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#newcode373
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#newcode528
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#newcode536
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.cc#newcode135
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.

http://codereview.appspot.com/5651069/