Re: Bugfix for issue 1630 (issue4490045)
2011/6/15 : > I've pushed a modified version of this. wOOt! YEEHAW! Hooray! > Given the confusion about our C++ style, I removed a few changes which > were probably good. In some places I added an extra (pointless) space; > in other places I changed a tab character to a space to match the > existing (broken) indentation, etc. The goal was to eliminate as many > changes as possible. > > I know that Karin and Janek worked hard on some of those style changes, > so I'm sorry for removing that work -- but I thought that avoiding ANY > change which was not strictly necessary would less the chance of anybody > finding something to complain about. meh. thanks for pushing!! Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
I've pushed a modified version of this. Given the confusion about our C++ style, I removed a few changes which were probably good. In some places I added an extra (pointless) space; in other places I changed a tab character to a space to match the existing (broken) indentation, etc. The goal was to eliminate as many changes as possible. I know that Karin and Janek worked hard on some of those style changes, so I'm sorry for removing that work -- but I thought that avoiding ANY change which was not strictly necessary would less the chance of anybody finding something to complain about. The resulting patch (which is now in git master) does not break any regtests, and it produces good output according to the example in comment 1 of http://code.google.com/p/lilypond/issues/detail?id=1630 I'm really sorry about all the style problems; we will begin sorting those out systematically starting on 22 June 2011. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On Wed, Jun 15, 2011 at 02:05:10PM +0200, Janek Warchoł wrote: > Patches attached. Rebased from 12 to 5 commits; i kept > "aaa!! indent" commit for historical reasons. I can > rebase more if you want. Nah, I'm fine. Rebased down to 1, including some more whitespace changes. Doing a final regtest check right now. > > This 68-line-patch has taken almost seven weeks. > > Shortened flags may break that record :) Final count: - 61 lines + 53 lines You'll probably be pissed off when you see how simple the final patch looks. I know that I would be. I can't wait until we've cleaned up the code style problems in git. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
2011/6/15 Graham Percival : > On Wed, Jun 15, 2011 at 01:02:07PM +0200, Janek Warchoł wrote: >> 2011/6/15 : >> > I've identified a few more questionable lines, although maybe you should >> > wait for a lilypond C++ expert to look at them. >> >> Well then, i'm stuffed. > > yeah. > > Enough screwing around. Send me the final patch. I'll play with > some indentation and push it today. If anybody doesn't like > whatever indentation I end up using, they can fix it in git > themselves. Whoah! Hooray for Graham! Patches attached. Rebased from 12 to 5 commits; i kept "aaa!! indent" commit for historical reasons. I can rebase more if you want. > This 68-line-patch has taken almost seven weeks. Shortened flags may break that record :) thanks! Janek issue 1630 patches rebased.tar.gz Description: GNU Zip compressed data ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On Wed, Jun 15, 2011 at 01:02:07PM +0200, Janek Warchoł wrote: > 2011/6/15 : > > I've identified a few more questionable lines, although maybe you should > > wait for a lilypond C++ expert to look at them. > > Well then, i'm stuffed. yeah. Enough screwing around. Send me the final patch. I'll play with some indentation and push it today. If anybody doesn't like whatever indentation I end up using, they can fix it in git themselves. This 68-line-patch has taken almost seven weeks. - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
W dniu 15 czerwca 2011 00:49 użytkownik Carl Sorensen napisał: > > On 6/14/11 4:19 PM, "Janek Warchoł" > wrote: > > I'm disheartened by the idea of reverting tab->space conversion, > > because CG 10.3.2 says "All indentation should be done with spaces". > > Strictly speaking, CG 10.3.2 applies to changes in .scm and .ly files (see > section 10.3). > > C++ standards are in CG 10.5; indentation standards are 10.5.3. Ah. they are written in a form completely unreadable to me (ofc i checked "GNU style" on Wikipedia, but it doesn't say whether tabs should be used or not). As tabs-vs-spaces issue wasn't adressed there i was sure that the statement from CG 10.3.2 applies everywhere. Thanks for explaining this to me. 2011/6/15 : > > "All indentation should be done with spaces". > > This is why I changed some tabs to spaces - sorry if this caused > confusion. Don't worry, the most confusion was caused by not having well-defined, global and easy-to-follow code indentation rules :P cheers, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
"All indentation should be done with spaces". This is why I changed some tabs to spaces - sorry if this caused confusion. I suggest that someone who knows the unwritten rules of indentation finalizes the format issues. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
2011/6/15 : > I've identified a few more questionable lines, although maybe you should > wait for a lilypond C++ expert to look at them. Well then, i'm stuffed. If you don't want to make an exception to the indentation rules for this issue, i'm not going to touch indentation here until GOP C++ formatting is settled (unless Karin's motivation requires it!). > this stuff is ridiculous. :/ Even more than that. Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
I've identified a few more questionable lines, although maybe you should
wait for a lilypond C++ expert to look at them.
Now do you see why I was apologizing so much for not starting the C++
GOP question this week? this stuff is ridiculous. :/
http://codereview.appspot.com/4490045/diff/35002/lily/tie-engraver.cc
File lily/tie-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/35002/lily/tie-engraver.cc#newcode157
lily/tie-engraver.cc:157: maybe should check positions too.
this should be indented with:
1 tab
http://codereview.appspot.com/4490045/diff/35002/lily/tie-engraver.cc#newcode189
lily/tie-engraver.cc:189: /*
this should be indented with:
1 tab
http://codereview.appspot.com/4490045/diff/35002/lily/tie-engraver.cc#newcode190
lily/tie-engraver.cc:190: Prevent all other tied notes ending at the
same moment (assume
this should be indented with:
1 tab + 2 spaces
http://codereview.appspot.com/4490045/diff/35002/lily/tie-engraver.cc#newcode238
lily/tie-engraver.cc:238: {
heh, wow, the original file was wrong. Go figure.
(yes, this change is correct)
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
indent indent indent http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc#newcode189 lily/tie-engraver.cc:189: /* On 2011/06/14 23:07:54, Graham Percival wrote: these should be tabs aaa!!! Done! But the indentation in this place is generally screwed, even besides tabs/spaces! !! http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/25003/lily/tie-engraver.cc#newcode189 lily/tie-engraver.cc:189: /* these should be tabs hint: look at the side-by-side comparison on rietveld. If you see a red symbol on the left-hand side, but no >> symbol on the right-hand side, then you've deleted a tab. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 6/14/11 4:19 PM, "Janek Warchoł" wrote: > 2011/6/14 >> and Janek: you are probably quite >> confused (and maybe disheartened) by this indentation problem, because >> the current indentation policy used in lilypond is completely braindead >> and ridiculous for any editor other than GNU/emacs. > > I'm disheartened by the idea of reverting tab->space conversion, > because CG 10.3.2 says "All indentation should be done with spaces". Strictly speaking, CG 10.3.2 applies to changes in .scm and .ly files (see section 10.3). The .ly file proposal is fully supported, AFAIK. The .scm proposal is moderately supported. But the current policy is "do what's in the current file." I hope we can get to "don't use tabs", but we haven't got there fully. > >> I have just postponed the "lessons from 2.14" GOP policy question for >> another week so that we can start dealing with "C++ indentation" on Wed >> 22 June. Unfortunately the discussion would not be over until 06 July, >> but that's the best I can do (unless I postpone the "mentors" >> discussion, but I think that one's even more pressing than indentation). > > No, don't postpone mentors! > >> If you want the patch pushed sooner, then unfortunately you need to use >> the exact (braindead) indentation we currently have for C++ code. >> Alternately, wait until we have a sensible policy > > I think we are inconsistent: sometimes we stick to the rules very > strictly, and sometimes not at all. I think that the C++ rules are quite strictly enforced, although I won't promise that all existing files follow them. C++ standards are in CG 10.5; indentation standards are 10.5.3. Previous discussions on tabs v. spaces include the following: http://thread.gmane.org/gmane.comp.gnu.lilypond.devel/22691 http://lists.gnu.org/archive/html/lilypond-devel/2009-04/msg00076.html It's part of GLISS, I think. Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
Thanks to Neil's help, it's done. http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 lily/tie-engraver.cc:157: maybe should check positions too. On 2011/06/14 21:36:59, Neil Puttock wrote: looks like tab -> space conversion going on here (and below); please restore and only fix bad indents Done. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On Wed, Jun 15, 2011 at 12:19:01AM +0200, Janek Warchoł wrote: > 2011/6/14 > > and Janek: you are probably quite > > confused (and maybe disheartened) by this indentation problem, because > > the current indentation policy used in lilypond is completely braindead > > and ridiculous for any editor other than GNU/emacs. > > I'm disheartened by the idea of reverting tab->space conversion, > because CG 10.3.2 says "All indentation should be done with spaces". ... hahahahahahahahaha... it's true, I just checked it. sob. - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
2011/6/14 > > http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 > lily/tie-engraver.cc:157: maybe should check positions too. > On 2011/06/14 21:36:59, Neil Puttock wrote: >> >> looks like tab -> space conversion going on here (and below); >> please restore and only fix bad indents > > Neil, you are absolutely correct (as always). > > However, I have a proposal for Karen it's Karin :) > and Janek: you are probably quite > confused (and maybe disheartened) by this indentation problem, because > the current indentation policy used in lilypond is completely braindead > and ridiculous for any editor other than GNU/emacs. I'm disheartened by the idea of reverting tab->space conversion, because CG 10.3.2 says "All indentation should be done with spaces". > I have just postponed the "lessons from 2.14" GOP policy question for > another week so that we can start dealing with "C++ indentation" on Wed > 22 June. Unfortunately the discussion would not be over until 06 July, > but that's the best I can do (unless I postpone the "mentors" > discussion, but I think that one's even more pressing than indentation). No, don't postpone mentors! > If you want the patch pushed sooner, then unfortunately you need to use > the exact (braindead) indentation we currently have for C++ code. > Alternately, wait until we have a sensible policy I think we are inconsistent: sometimes we stick to the rules very strictly, and sometimes not at all. Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 14 June 2011 22:57, Graham Percival wrote: > On Tue, Jun 14, 2011 at 09:43:56PM +, n.putt...@gmail.com wrote: >> To save time fixing indentation, if you have emacs installed there's a >> script which will nitpick the code: >> >> scripts/auxiliar/fixcc.py >> >> http://lilypond.org/doc/v2.15/Documentation/contributor/indentation > > Unfortunately that script is out of date with the code (or vice > versa); running the script on every .h and .cc file in git > produces a diff which is something like 500 Kb. I wouldn't advise using it on headers, but it's usually OK inside lily/ (apart from the annoying reformatting in the documentation macro) > That said, it might work on completion-note-heads-engraver.cc. > It's certainly worth a shot. It's fine here. Cheers, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On Tue, Jun 14, 2011 at 09:43:56PM +, n.putt...@gmail.com wrote: > To save time fixing indentation, if you have emacs installed there's a > script which will nitpick the code: > > scripts/auxiliar/fixcc.py > > http://lilypond.org/doc/v2.15/Documentation/contributor/indentation Unfortunately that script is out of date with the code (or vice versa); running the script on every .h and .cc file in git produces a diff which is something like 500 Kb. That said, it might work on completion-note-heads-engraver.cc. It's certainly worth a shot. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 lily/tie-engraver.cc:157: maybe should check positions too. On 2011/06/14 21:36:59, Neil Puttock wrote: looks like tab -> space conversion going on here (and below); please restore and only fix bad indents Neil, you are absolutely correct (as always). However, I have a proposal for Karen and Janek: you are probably quite confused (and maybe disheartened) by this indentation problem, because the current indentation policy used in lilypond is completely braindead and ridiculous for any editor other than GNU/emacs. I have just postponed the "lessons from 2.14" GOP policy question for another week so that we can start dealing with "C++ indentation" on Wed 22 June. Unfortunately the discussion would not be over until 06 July, but that's the best I can do (unless I postpone the "mentors" discussion, but I think that one's even more pressing than indentation). If you want the patch pushed sooner, then unfortunately you need to use the exact (braindead) indentation we currently have for C++ code. Alternately, wait until we have a sensible policy, backed up by automatic formatting so that no human ever needs to fiddle with C++ indentation again because it's 2011 and it's silly to do this by hand. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
Hi Janek, To save time fixing indentation, if you have emacs installed there's a script which will nitpick the code: scripts/auxiliar/fixcc.py http://lilypond.org/doc/v2.15/Documentation/contributor/indentation Cheers, Neil http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode157 lily/tie-engraver.cc:157: maybe should check positions too. looks like tab -> space conversion going on here (and below); please restore and only fix bad indents http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode168 lily/tie-engraver.cc:168: && (!Tie_engraver::has_autosplit_end (left_ev))) indent: if (ly_is_equal && ( http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode292 lily/tie-engraver.cc:292: && (!Tie_engraver::has_autosplit_end (left_ev))) indent: if (left_ev && ( http://codereview.appspot.com/4490045/diff/30001/lily/tie-engraver.cc#newcode296 lily/tie-engraver.cc:296: Head_event_tuple event_tup; line up with event_processed = true; (and following lines) http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
2011/6/14 : > > On 2011/06/14 20:28:14, Janek Warchol wrote: >> New patch set uploaded. >> Should i run the regtests again? > > LGTM, but yes please, run the regtests. Regtests clean. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/06/14 20:28:14, Janek Warchol wrote: New patch set uploaded, i think all formatting issues are resolved. Should i run the regtests again? LGTM, but yes please, run the regtests. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
New patch set uploaded, i think all formatting issues are resolved.
Should i run the regtests again?
http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc#newcode204
lily/completion-note-heads-engraver.cc:204:
event->set_property("autosplit-end",
On 2011/06/01 21:21:25, Neil Puttock wrote:
set_property (
Done.
http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc#newcode205
lily/completion-note-heads-engraver.cc:205: ly_bool2scm (left_to_do_ -
note_dur.get_length () > Rational (0)));
On 2011/06/01 21:21:25, Neil Puttock wrote:
indent:
event->set_property ("autosplit-end",
ly_bool2scm (
Done.
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc
File lily/tie-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode85
lily/tie-engraver.cc:85: bool has_autosplit_end (Stream_event* event);
On 2011/06/01 21:21:25, Neil Puttock wrote:
Stream_event *event
Done.
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode125
lily/tie-engraver.cc:125: Tie_engraver::has_autosplit_end (Stream_event*
event)
On 2011/06/01 21:21:25, Neil Puttock wrote:
Stream_event *event
Done.
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode163
lily/tie-engraver.cc:163: /*
On 2011/06/13 22:51:39, Graham Percival wrote:
On 2011/06/13 22:44:37, karin.hoethker wrote:
> On 2011/06/01 21:21:25, Neil Puttock wrote:
> > indent
>
> I don't see a problem here.
The /* should line up with the "if" on line 159. Line 167 should also
line up
with line 159.
Done.
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode163
lily/tie-engraver.cc:163: /*
On 2011/06/01 21:21:25, Neil Puttock wrote:
indent
Done.
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode289
lily/tie-engraver.cc:289: if (left_ev && (tie_event || tie_stream_event)
On 2011/06/01 21:21:25, Neil Puttock wrote:
indent
Done.
http://codereview.appspot.com/4490045/diff/20001/scm/define-music-properties.scm
File scm/define-music-properties.scm (right):
http://codereview.appspot.com/4490045/diff/20001/scm/define-music-properties.scm#newcode44
scm/define-music-properties.scm:44: (autosplit-end ,boolean? "Duration
of event was truncated by automatic splitting in
Completion_heads_engraver.")
On 2011/06/01 21:21:25, Neil Puttock wrote:
the @code{Completion_heads_engraver}.
Done.
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode163 lily/tie-engraver.cc:163: /* On 2011/06/13 22:44:37, karin.hoethker wrote: On 2011/06/01 21:21:25, Neil Puttock wrote: > indent I don't see a problem here. The /* should line up with the "if" on line 159. Line 167 should also line up with line 159. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
I have send the patch set to Janek for upload. http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode163 lily/tie-engraver.cc:163: /* On 2011/06/01 21:21:25, Neil Puttock wrote: indent I don't see a problem here. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
LGTM.
http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc#newcode204
lily/completion-note-heads-engraver.cc:204:
event->set_property("autosplit-end",
set_property (
http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-engraver.cc#newcode205
lily/completion-note-heads-engraver.cc:205: ly_bool2scm (left_to_do_ -
note_dur.get_length () > Rational (0)));
indent:
event->set_property ("autosplit-end",
ly_bool2scm (
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc
File lily/tie-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode85
lily/tie-engraver.cc:85: bool has_autosplit_end (Stream_event* event);
Stream_event *event
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode125
lily/tie-engraver.cc:125: Tie_engraver::has_autosplit_end (Stream_event*
event)
Stream_event *event
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode163
lily/tie-engraver.cc:163: /*
indent
http://codereview.appspot.com/4490045/diff/20001/lily/tie-engraver.cc#newcode289
lily/tie-engraver.cc:289: if (left_ev && (tie_event || tie_stream_event)
indent
http://codereview.appspot.com/4490045/diff/20001/scm/define-music-properties.scm
File scm/define-music-properties.scm (right):
http://codereview.appspot.com/4490045/diff/20001/scm/define-music-properties.scm#newcode44
scm/define-music-properties.scm:44: (autosplit-end ,boolean? "Duration
of event was truncated by automatic splitting in
Completion_heads_engraver.")
the @code{Completion_heads_engraver}.
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
LGTM Carl http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc#newcode207
lily/completion-note-heads-engraver.cc:207:
event->set_property("autosplit-end", ly_bool2scm (false));
On 2011/05/28 20:27:13, karin.hoethker wrote:
However, some people consider inline conditions not to be
very readable (for a (Java) example see AvoidInlineConditionals at
http://checkstyle.sourceforge.net/config_coding.html). Although this
is not an
extreme case, I'd rather vote for readability and leave the code as it
is.
FWIW, I prefer Karin's formulation:
if (blah) func(true); else func(false);
However, David and Pal combined have at least 10 times as much
experience with lilypond C++ code as I do, so I think it would be best
if this conditional were changed.
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/05/29 18:36:20, Janek Warchol wrote: New patch set from Karin uploaded. thanks, looks marvelous to me. Pal http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
New patch set from Karin uploaded. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
karin.hoeth...@googlemail.com writes: > I just used inline conditionals as an example of a code style where > conditions are inlined. More generally, there seem to be two views on > readability: > > One could be summarized as "Don't do more than one thing in one line" > (for example inline conditions that might come as evaluation of a > boolean expression within an assignment, or inline conditionals). The > code is easier to "parse" for a new reader and easier to debug. This > view seems to be popular in the Java world. > > The other take is "Compact code is more readable" as in the proposed > change. If an experienced reader recognizes patterns such as "Boolean > evaluation inside an argument" the code may be faster to read (unless > the code is too compact). We are not talking about compact code here but code duplication. Since the duplication occurs in different execution paths, it is not per se a performance issue (though branch prediction can be expensive nowadays), but it means having to read both branches to figure out what is doing done. It also means having to do a complex text comparison to figure out how the two different code paths differ. Personally, I find having to parse things like if (condition != false) then somethingcomplicated(true) else somethingcomplicated(false) annoying and contorted. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/05/29 12:14:18, Carl wrote: On 2011/05/29 08:53:30, benko.pal wrote: > I must miss something, to me it's still a boolean. > and (still to me) it's not an inline conditional, but > an assignment of a boolean expression to a boolean > variable. I just used inline conditionals as an example of a code style where conditions are inlined. More generally, there seem to be two views on readability: One could be summarized as "Don't do more than one thing in one line" (for example inline conditions that might come as evaluation of a boolean expression within an assignment, or inline conditionals). The code is easier to "parse" for a new reader and easier to debug. This view seems to be popular in the Java world. The other take is "Compact code is more readable" as in the proposed change. If an experienced reader recognizes patterns such as "Boolean evaluation inside an argument" the code may be faster to read (unless the code is too compact). If it helps to close the issue I'll change the code. I suppose that the general question will be settled in the upcoming style discussion. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
2011/5/28 :
> On 2011/05/28 16:13:43, benko.pal wrote:
>>
>> aargh, that's not too readable.
>> what I actually suggest is replacing lines 204-207 of
>
>> >
>
> http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc
>>
>> > File lily/completion-note-heads-engraver.cc (right):
>
>> 204 if ((left_to_do_ - note_dur.get_length ()) > Rational (0))
>> 205 event->set_property("autosplit-end", ly_bool2scm (true));
>> 206 else
>> 207 event->set_property("autosplit-end", ly_bool2scm (false));
>
>> by
>
>> event->set_property ("autosplit-end",
>> ly_bool2scm (left_to_do_ - note_dur.get_length () >
>
> 0));
>
>> Pal
>
> That was the original code. It was pointed out (see Neil's comment
> above) that the only check on this is whether or not it is greater than
> zero, so a boolean works. Hence, the code was changed to use a boolean.
I must miss something, to me it's still a boolean.
and (still to me) it's not an inline conditional, but
an assignment of a boolean expression to a boolean
variable.
to me the idiom
if (complex_boolean_expression)
foo(true);
else
foo(false);
is more cluttersome than
foo(complex_boolean_expression);
but if that's a minority opinion, then leave Karin's
code as is, of course.
> http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/05/29 08:53:30, benko.pal wrote: I must miss something, to me it's still a boolean. and (still to me) it's not an inline conditional, but an assignment of a boolean expression to a boolean variable. No, it is I that missed something. I'm sorry for the noise. Thanks, Carl http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc#newcode207
lily/completion-note-heads-engraver.cc:207:
event->set_property("autosplit-end", ly_bool2scm (false));
The style guide in Lilypond Contributor's Guide
(http://lilypond.org/doc/v2.13/Documentation/contributor/code-style)
does not cover the question. However, some people consider inline
conditions not to be very readable (for a (Java) example see
AvoidInlineConditionals at
http://checkstyle.sourceforge.net/config_coding.html). Although this is
not an extreme case, I'd rather vote for readability and leave the code
as it is.
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
aargh, that's not too readable.
what I actually suggest is replacing lines 204-207 of
> http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc
> File lily/completion-note-heads-engraver.cc (right):
204 if ((left_to_do_ - note_dur.get_length ()) > Rational (0))
205 event->set_property("autosplit-end", ly_bool2scm (true));
206 else
207 event->set_property("autosplit-end", ly_bool2scm (false));
by
event->set_property ("autosplit-end",
ly_bool2scm (left_to_do_ - note_dur.get_length () > 0));
Pal
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
New patches from Karin uploaded.
I've got a warning while applying patch 0003 (about autosplit-remainder)
to my repository, but it looks like its not fatal... I don't understand
what it means, can anyone more experienced take a look and say whether
there is anything to worry about?
janek@janek-virtual4:~/lilypond-git$ git am
0003-Issue-1630-autosplit-remainder-renamed-to-boolean-au.patch
Applying: Issue 1630: autosplit-remainder renamed to boolean
autosplit-end
/home/janek/lilypond-git/.git/rebase-apply/patch:19: trailing
whitespace.
was truncated during splitting. Based on "autosplit-end", the
Tie_engraver decides whether a
/home/janek/lilypond-git/.git/rebase-apply/patch:26: trailing
whitespace.
event->set_property("autosplit-end", ly_bool2scm (false));
warning: 2 lines add whitespace errors.
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/05/28 16:13:43, benko.pal wrote:
aargh, that's not too readable.
what I actually suggest is replacing lines 204-207 of
>
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc
> File lily/completion-note-heads-engraver.cc (right):
204 if ((left_to_do_ - note_dur.get_length ()) > Rational (0))
205 event->set_property("autosplit-end", ly_bool2scm (true));
206 else
207 event->set_property("autosplit-end", ly_bool2scm (false));
by
event->set_property ("autosplit-end",
ly_bool2scm (left_to_do_ - note_dur.get_length () >
0));
Pal
That was the original code. It was pointed out (see Neil's comment
above) that the only check on this is whether or not it is greater than
zero, so a boolean works. Hence, the code was changed to use a boolean.
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc#newcode207
lily/completion-note-heads-engraver.cc:207:
event->set_property("autosplit-end", ly_bool2scm (false));
event->set_property ("autosplit-end",
ly_bool2scm (left_to_do_ - note_dur.get_length () > 0));
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/05/25 13:25:27, karin.hoethker wrote:
You are right, in version 2.13 even shifted tuplets like "c'8 \times
2/3 { c'4
c'4 c'4} c'4." are split into correct durations.
It seems to me that people are rather in favour of the shorter
solution using a
boolean flag instead of the duration "autosplit-remainder".
For clarity, Karin: there are some issues with this patch, so we are
waiting for a new draft from you. If you have difficulty uploading the
patch, please ask Janek.
(I'm not trying to rush you or anything; I just want to be certain that
everybody knows where we stand with this patch)
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
You are right, in version 2.13 even shifted tuplets like "c'8 \times 2/3
{ c'4 c'4 c'4} c'4." are split into correct durations.
It seems to me that people are rather in favour of the shorter solution
using a boolean flag instead of the duration "autosplit-remainder".
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
2011/5/9 :
> http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc
> File lily/completion-note-heads-engraver.cc (right):
>
> http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204
> lily/completion-note-heads-engraver.cc:204:
> event->set_property("autosplit-remainder", Moment(left_to_do_ -
> note_dur.get_length ()).smobbed_copy ());
> Indeed, using a boolean would be the minimal solution. However, the
> remaining duration is related to the current time and the overall note
> duration etc. If for example tuplet splitting was to be implemented for
> the Completions_heads_engraver, the remainder could be useful in a
> splitting strategy.
note that minimal tuplet splitting is supported, see regression test
completion-heads-tuplets.ly. there are lots more to do, but perhaps
not in the Completion_heads_engraver.
p
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204
lily/completion-note-heads-engraver.cc:204:
event->set_property("autosplit-remainder", Moment(left_to_do_ -
note_dur.get_length ()).smobbed_copy ());
Indeed, using a boolean would be the minimal solution. However, the
remaining duration is related to the current time and the overall note
duration etc. If for example tuplet splitting was to be implemented for
the Completions_heads_engraver, the remainder could be useful in a
splitting strategy.
It depends on whether minimal data structures or minimal renaming of
variables is preferred in lilypond development. I don't know about that
...
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204
lily/completion-note-heads-engraver.cc:204:
event->set_property("autosplit-remainder", Moment(left_to_do_ -
note_dur.get_length ()).smobbed_copy ());
Wouldn't this work equally well as a boolean?
As far as I can see, the Tie_engraver only reads this property, checking
whether main_part_ is > 0, so this test could be done here instead,
e.g.,
if (Moment (left_to_do_ - note_dur.get_length)).main_part_)
event->set_property ("autosplit-remainder", SCM_BOOL_T);
(obviously this would mean renaming this property)
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc
File lily/tie-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode121
lily/tie-engraver.cc:121: Determines whether the end of an event was
created by
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode168
lily/tie-engraver.cc:168: Make a tie only if pitches are equal or if
event end was not generated by
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode172
lily/tie-engraver.cc:172: &&
(!Tie_engraver::has_autosplit_end(left_ev)))
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode261
lily/tie-engraver.cc:261: */
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode294
lily/tie-engraver.cc:294: &&
(!Tie_engraver::has_autosplit_end(left_ev)))
indent
space before (left_ev)
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
LGTM, except for non-standard indentation which should be
corrected, although I've tested it only on the examples in
the regression tests.
Trevor
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode92
lily/completion-note-heads-engraver.cc:92: is_first_ = false;
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc
File lily/tie-engraver.cc (right):
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode126
lily/tie-engraver.cc:126: {
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode128
lily/tie-engraver.cc:128: {
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode297
lily/tie-engraver.cc:297: event_processed = true;
indent
http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode341
lily/tie-engraver.cc:341: event_ = 0;
indent
http://codereview.appspot.com/4490045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
I've checked that Karin's patch set passes regression tests successfully. cheers, Janek 2011/5/7 > Reviewers: karin_hoethker.de, Graham Percival, lemzwerg, > carl.d.sorensen_gmail.com, > > Message: > Finally i'm back in Warsaw and i'm uploading Karin's patch. Sorry for > the delay :( > > Here's an archive with test file and output pdfs from before and after > the patch: http://www.sendspace.com/file/gog5zh > > I see that Carl's comments written in issue tracker are now resolved > (commenting style and scm properties), so the patch is ready for > discussion i think. > > Again, my apologies for the delay! > > Janek > > Description: > Bugfix for issue 1630 > > Remove duplicate ties for chords autosplit with > Completion_heads_engraver and manually tied. > > Please review this at http://codereview.appspot.com/4490045/ > > Affected files: > M lily/completion-note-heads-engraver.cc > M lily/tie-engraver.cc > M scm/define-music-properties.scm > > > ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Bugfix for issue 1630 (issue4490045)
Reviewers: karin_hoethker.de, Graham Percival, lemzwerg, carl.d.sorensen_gmail.com, Message: Finally i'm back in Warsaw and i'm uploading Karin's patch. Sorry for the delay :( Here's an archive with test file and output pdfs from before and after the patch: http://www.sendspace.com/file/gog5zh I see that Carl's comments written in issue tracker are now resolved (commenting style and scm properties), so the patch is ready for discussion i think. Again, my apologies for the delay! Janek Description: Bugfix for issue 1630 Remove duplicate ties for chords autosplit with Completion_heads_engraver and manually tied. Please review this at http://codereview.appspot.com/4490045/ Affected files: M lily/completion-note-heads-engraver.cc M lily/tie-engraver.cc M scm/define-music-properties.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
