2310 Corrected style of comments

Corrected style of comments

[Milimetr88, 2012-03-21 18:55:31]

This is a continuation of Issue: http://codereview.appspot.com/5651069/

[Julien Rioux, 2012-03-22 14:43:30]

Are the changes to .gitignore intentional?

[Graham Percival, 2012-03-22 14:55:38]

I really like seeing work on comments.  Have you experimented with doxygen?  If
you're going to be correcting comments, it might be good to "go the full
distance" and make sure that the new comments produce good doxygen docs. 
Long-term (say, 3-4 months) we could even wrap doxygen into the build system and
add that info to the internals reference; this could be a great help to new
programmers.

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#new...
lily/accidental-placement.cc:116: //TODO: add comment to this class
I'm not certain this line adds anything.  Also, isn't it a struct, not a class?

[Keith, 2012-03-23 07:23:36]

This patch adds helpful comments.

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#newc...
flower/include/direction.hh:77: * Thanks to a #define below, instead of writing:
> 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)

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#newcode183
lily/note-collision.cc:183: */
Move the block comment above down below your new comments, to keep it adjacent
to the if...elseif... block that it describes.

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)
What does "the offset" refer to?  shift_amount?

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,
> What I was taught on the university is to write short 
>  and simple functions that do only one thing. 

In this case, the function is too short, relative to the complexity of the
interface(*), for separation. 

(*) The function changes what is pointed to by its first argument, which is not
unusual, but milimetre88 found the same operation in another function worth a
comment with a '!'. 
The second argument is used only for its dimensions, which are related to the
dimensions of the first argument for reasons we can understand only by looking
at the calling function.  I would be confused wondering why the function does
not simply use the dimensions of offsets[][].

[Milimetr88, 2012-03-23 10:41:48]

On 22 March 2012 15:43, <julien.rioux@gmail.com> wrote:

> Are the changes to .gitignore intentional?
>
>
http://codereview.appspot.com/**5862052/<http://codereview.appspot.com/5862052/>
>

Ooops, changes - yes, placing them in the patch - of course no.

Łukasz

[Milimetr88, 2012-03-31 16:13:11]

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#u...
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#newc...
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#new...
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!

[janek, 2012-03-31 20:02:28]

On Sat, Mar 31, 2012 at 6:13 PM,  <milimetr88@gmail.com> wrote:
> 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#u...
> 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'?

The information about Rietveld issue is stored in your git config, in
the properties of the branch you used when you uploaded first.  If you
upload new patch revision from a new branch, the script doesn't know
what rietveld number is because it doesn't see it in branch
properties.  It's a good practice to use one branch for your fix.

HTH,
Janek

[pkx166h, 2012-03-31 20:46:07]

Hello,

2012/3/31 Janek Warchoł <janek.lilypond@gmail.com>:
> On Sat, Mar 31, 2012 at 6:13 PM,  <milimetr88@gmail.com> wrote:
>> 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#u...
>> 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'?
>
> The information about Rietveld issue is stored in your git config, in
> the properties of the branch you used when you uploaded first.  If you
> upload new patch revision from a new branch, the script doesn't know
> what rietveld number is because it doesn't see it in branch
> properties.  It's a good practice to use one branch for your fix.

I find that using 'git-cl issue 0' helps to 'reset' any doubts abuot
what git-cl thinks it has currently. I use a VM with Lilydev and have
a number of patches on the go, so forget which is which, so just reset
git-cl and then either let git-cl create my new issue number or I find
my rietveld issue and then run git-cl issue XXXX where XXXX is the
rietveld number. Then I know I am good to go.

James

[Milimetr88, 2012-04-01 08:46:22]

Thanku you, Janek and James.

Wouldn't it be nicer for git cl to ask for an issue number in case it is
unknown? Default value (e.g. 0) could mean a new issue, but the programmer
would have an opportunity to type the right issue number.

Łukasz


On 31 March 2012 22:46, James <pkx166h@gmail.com> wrote:

> Hello,
>
> 2012/3/31 Janek Warchoł <janek.lilypond@gmail.com>:
> > On Sat, Mar 31, 2012 at 6:13 PM,  <milimetr88@gmail.com> wrote:
> >> 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#u...
> >> 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'?
> >
> > The information about Rietveld issue is stored in your git config, in
> > the properties of the branch you used when you uploaded first.  If you
> > upload new patch revision from a new branch, the script doesn't know
> > what rietveld number is because it doesn't see it in branch
> > properties.  It's a good practice to use one branch for your fix.
>
> I find that using 'git-cl issue 0' helps to 'reset' any doubts abuot
> what git-cl thinks it has currently. I use a VM with Lilydev and have
> a number of patches on the go, so forget which is which, so just reset
> git-cl and then either let git-cl create my new issue number or I find
> my rietveld issue and then run git-cl issue XXXX where XXXX is the
> rietveld number. Then I know I am good to go.
>
> James
>

[Graham Percival, 2012-04-01 08:57:14]

On Sun, Apr 01, 2012 at 10:46:01AM +0200, Łukasz Czerwiński wrote:
>    Thanku you, Janek and James.

Don't add replies to the top an email.

>    Wouldn't it be nicer for git cl to ask for an issue number in case it
>    is unknown?

It does.  I suggest you update your git-cl to get this addition
(which was made about 4 months ago?).

- Graham

[janek, 2012-04-01 09:01:04]

On Sun, Apr 1, 2012 at 10:57 AM, Graham Percival
<graham@percival-music.ca> wrote:
> On Sun, Apr 01, 2012 at 10:46:01AM +0200, Łukasz Czerwiński wrote:
>>    Thanku you, Janek and James.
>
> Don't add replies to the top an email.
>
>>    Wouldn't it be nicer for git cl to ask for an issue number in case it
>>    is unknown?
>
> It does.  I suggest you update your git-cl to get this addition
> (which was made about 4 months ago?).

I suppose he means asking about Rietveld issue number, not tracker
issue number.  IIRC git cl only asks about the latter.

Janek

[Graham Percival, 2012-04-01 09:21:39]

On Sun, Apr 01, 2012 at 11:00:44AM +0200, Janek Warchoł wrote:
> On Sun, Apr 1, 2012 at 10:57 AM, Graham Percival
> <graham@percival-music.ca> wrote:
> > On Sun, Apr 01, 2012 at 10:46:01AM +0200, Łukasz Czerwiński wrote:
> >>    Wouldn't it be nicer for git cl to ask for an issue number in case it
> >>    is unknown?
> >
> > It does.  I suggest you update your git-cl to get this addition
> > (which was made about 4 months ago?).
> 
> I suppose he means asking about Rietveld issue number, not tracker
> issue number.  IIRC git cl only asks about the latter.

oh, I see.  Sorry, my mistake.

If you're sending patches from different computers, I think you
can do it with
  git-cl issue 1234
  git-cl upload

- Graham

[mail_philholmes.net, 2012-04-01 10:10:07]

----- Original Message ----- 
From: "Graham Percival" <graham@percival-music.ca>
To: "Janek Warchoł" <janek.lilypond@gmail.com>
Cc: <k-ohara5a5a@oco.net>; <reply@codereview-hr.appspotmail.com>; 
<julien.rioux@gmail.com>; <lilypond-devel@gnu.org>
Sent: Sunday, April 01, 2012 10:21 AM
Subject: Re: Corrected style of comments (issue 5862052)


> On Sun, Apr 01, 2012 at 11:00:44AM +0200, Janek Warchoł wrote:
>> On Sun, Apr 1, 2012 at 10:57 AM, Graham Percival
>> <graham@percival-music.ca> wrote:
>> > On Sun, Apr 01, 2012 at 10:46:01AM +0200, Łukasz Czerwiński wrote:
>> >> Wouldn't it be nicer for git cl to ask for an issue number in case it
>> >> is unknown?
>> >
>> > It does. I suggest you update your git-cl to get this addition
>> > (which was made about 4 months ago?).
>>
>> I suppose he means asking about Rietveld issue number, not tracker
>> issue number.  IIRC git cl only asks about the latter.
>
> oh, I see.  Sorry, my mistake.
>
> If you're sending patches from different computers, I think you
> can do it with
>  git-cl issue 1234
>  git-cl upload
>
> - Graham

My recollection is that it assumes that the current issue number is the most 
recent you've uploaded to Rietveld.  This is great if you're only working on 
one.  If you're working on a number you have to use git cl issue xxxx to 
tell it that to use a different one.  git cl issue 0 tells it to create a 
new one.  All quite understandable and usable behaviour.


--
Phil Holmes

[Graham Percival, 2012-04-01 10:17:15]

On Sun, Apr 01, 2012 at 11:09:59AM +0100, Phil Holmes wrote:
> ----- Original Message ----- From: "Graham Percival"
> <graham@percival-music.ca>
> To: "Janek Warchoł" <janek.lilypond@gmail.com>
> Cc: <k-ohara5a5a@oco.net>; <reply@codereview-hr.appspotmail.com>;
> <julien.rioux@gmail.com>; <lilypond-devel@gnu.org>
> Sent: Sunday, April 01, 2012 10:21 AM
> Subject: Re: Corrected style of comments (issue 5862052)
> 
> >If you're sending patches from different computers, I think you
> >can do it with
> > git-cl issue 1234
> > git-cl upload
> 
> My recollection is that it assumes that the current issue number is
> the most recent you've uploaded to Rietveld.

It supposed to track it on a per-branch basis.  That's one reason
why the revised CG pushes (no pun intended) branches so heavily.

I mean, the CG now even covers branches in the "git for the
impatient", which is my absolutely shortest list of "I don't want
to learn anything about git; just give me commands to copy&paste
so I can improve lilypond".

- Graham