Re: Corrected style of comments (issue 5862052)
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 pkx1...@gmail.com wrote: Hello, 2012/3/31 Janek Warchoł janek.lilyp...@gmail.com: On Sat, Mar 31, 2012 at 6:13 PM, milimet...@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#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'? 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 where is the rietveld number. Then I know I am good to go. James ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
On Sun, Apr 1, 2012 at 10:57 AM, Graham Percival gra...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
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 gra...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
- Original Message - From: Graham Percival gra...@percival-music.ca To: Janek Warchoł janek.lilyp...@gmail.com Cc: k-ohara5...@oco.net; re...@codereview-hr.appspotmail.com; julien.ri...@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 gra...@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 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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
On Sun, Apr 01, 2012 at 11:09:59AM +0100, Phil Holmes wrote: - Original Message - From: Graham Percival gra...@percival-music.ca To: Janek Warchoł janek.lilyp...@gmail.com Cc: k-ohara5...@oco.net; re...@codereview-hr.appspotmail.com; julien.ri...@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 copypaste so I can improve lilypond. - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
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_arrayvectorReal
*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/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
On Sat, Mar 31, 2012 at 6:13 PM, milimet...@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#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'? 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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
Hello, 2012/3/31 Janek Warchoł janek.lilyp...@gmail.com: On Sat, Mar 31, 2012 at 6:13 PM, milimet...@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#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'? 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 where is the rietveld number. Then I know I am good to go. James ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
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#newcode77
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_arrayvectorReal
*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[][].
http://codereview.appspot.com/5862052/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
On 22 March 2012 15:43, julien.ri...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrected style of comments (issue 5862052)
Are the changes to .gitignore intentional? http://codereview.appspot.com/5862052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Corrected style of comments (issue 5862052)
Reviewers: , Message: This is a continuation of Issue: http://codereview.appspot.com/5651069/ Description: Corrected style of comments Please review this at http://codereview.appspot.com/5862052/ Affected files: M .gitignore M flower/include/direction.hh M lily/accidental-placement.cc M lily/grob.cc M lily/include/grob-interface.hh M lily/include/note-collision.hh M lily/note-collision.cc M lily/note-column.cc M lily/staff-symbol-referencer.cc ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
