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

[Carl . D . Sorensen]

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/