Re: Corrected style of comments (issue 5862052)

[milimetr88]

I'm sorry for that, but the next bunch of corrections are in a separate
issue: http://codereview.appspot.com/5975054

It seems that following the procedure from
http://lilypond.org/doc/v2.15/Documentation/contributor/commits-and-patches#uploading-a-patch-for-review
does not make git-cl ask for the issue number. Instead it creates a new
one. How to change that? Always call 'git cl issue' before 'git cl
upload'?


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:
On 2012/03/23 07:23:36, Keith wrote:
> > 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)

Yes, you are right - I should have changed all occurences at once. There
are 136 such. Do you have an idea, if it could be done automatically?

http://codereview.appspot.com/5862052/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5862052/diff/1/lily/accidental-placement.cc#newcode116
lily/accidental-placement.cc:116: //TODO: add comment to this class
On 2012/03/22 14:55:38, Graham Percival wrote:
> I'm not certain this line adds anything.  Also, isn't it a struct, not
a class?

Well, I think it adds something - a request to comment a struct that is
not obvious one.
Yes, it's a struct, not a class. AFAIR it's not a big difference in C++,
but ok, I'll correct the comment.

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#newcode76
lily/note-collision.cc:76: /* Filter out the 'o's in this configuration,
since they're no
Keith, I'm trying to understand the whole function and divide it into
parts.
Does this comment refer only to the two lines below?

http://codereview.appspot.com/5862052/diff/1/lily/note-collision.cc#newcode183
lily/note-collision.cc:183: */
On 2012/03/23 07:23:36, Keith wrote:
> Move the block comment above down below your new comments, to keep it
adjacent
> to the if...elseif... block that it describes.

Done.

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)
On 2012/03/23 07:23:36, Keith wrote:
> What does "the offset" refer to?  shift_amount?

Yes. Corrected.

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,
Well, ok, the function is indeed too short. But the whole function
check_meshing_chords should be split, don't you think? Currently it has
over 300 lines!

http://codereview.appspot.com/5862052/