Issue 4490045: Bugfix for issue 1630 (Closed)
|
Created:
13 years, 11 months ago by Janek Warchol Modified:
13 years, 10 months ago Reviewers:
Graham Percival, carl.d.sorensen, Neil Puttock, c_sorensen, lemzwerg, Trevor Daniels, benko.pal, karin.hoethker, karin, Graham Percival (old account) CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionBugfix for issue 1630
Remove duplicate ties for chords autosplit with Completion_heads_engraver and manually tied.
Patch Set 1 #
Total comments: 12
Patch Set 2 : fixing whitespace and changing autosplit remainder #
Total comments: 3
Patch Set 3 : Compact assignment to autosplit-end #
Total comments: 17
Patch Set 4 : fixing style #Patch Set 5 : more style fixing #
Total comments: 6
Patch Set 6 : indent issues #
Total comments: 2
Patch Set 7 : indent #Patch Set 8 : more indent! #Patch Set 9 : even more indent! I don't know what's going on here! #
Total comments: 4
MessagesTotal messages: 48
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
Sign in to reply to this message.
I've checked that Karin's patch set passes regression tests successfully. cheers, Janek 2011/5/7 <lemniskata.bernoullego@gmail.com> > 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 > > >
Sign in to reply to this message.
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-engra... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engra... 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
Sign in to reply to this message.
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engra... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engra... 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)
Sign in to reply to this message.
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engra... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engra... 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 ...
Sign in to reply to this message.
2011/5/9 <karin.hoethker@googlemail.com>: > http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engra... > File lily/completion-note-heads-engraver.cc (right): > > http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engra... > 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
Sign in to reply to this message.
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".
Sign in to reply to this message.
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)
Sign in to reply to this message.
Hi Graham, I sent two patches by mail to Janek yesterday, because I could not upload them to Rietveld myself. The first patch corrects the indentation problems, the second one changes the new parameter to type boolean. At the moment, I don't see further issues. Cheers, Karin Am 26.05.2011 14:30, schrieb percival.music.ca@gmail.com: > 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/ -- Karin Höthker, karin@hoethker.de write music together - www.scorio.com
Sign in to reply to this message.
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.
Sign in to reply to this message.
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-e... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-e... 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));
Sign in to reply to this message.
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-e... > 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
Sign in to reply to this message.
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-e... > > 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.
Sign in to reply to this message.
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-e... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-e... 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.
Sign in to reply to this message.
2011/5/28 <Carl.D.Sorensen@gmail.com>: > 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-e... >> >> > 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/
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-e... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-e... 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.
Sign in to reply to this message.
New patch set from Karin uploaded.
Sign in to reply to this message.
LGTM Carl
Sign in to reply to this message.
On 2011/05/29 18:36:20, Janek Warchol wrote: > New patch set from Karin uploaded. thanks, looks marvelous to me. Pal
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
LGTM. http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-e... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-e... 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-e... 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/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}.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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-e... File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/20001/lily/completion-note-heads-e... 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-e... 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/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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
2011/6/14 <percival.music.ca@gmail.com>: > > 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/
Sign in to reply to this message.
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)
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
On Tue, Jun 14, 2011 at 09:43:56PM +0000, n.puttock@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
Sign in to reply to this message.
On 14 June 2011 22:57, Graham Percival <graham@percival-music.ca> wrote: > On Tue, Jun 14, 2011 at 09:43:56PM +0000, n.puttock@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
Sign in to reply to this message.
2011/6/14 <percival.music.ca@gmail.com> > > 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
Sign in to reply to this message.
On Wed, Jun 15, 2011 at 12:19:01AM +0200, Janek Warchoł wrote: > 2011/6/14 <percival.music.ca@gmail.com> > > 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
Sign in to reply to this message.
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.
Sign in to reply to this message.
On 6/14/11 4:19 PM, "Janek Warchoł" <lemniskata.bernoullego@gmail.com> wrote: > 2011/6/14 <percival.music.ca@gmail.com> >> 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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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 aaaaaaaaaaaaaaaaaaaaaaa!!! Done! But the indentation in this place is generally screwed, even besides tabs/spaces! aaaaaaaaaaaaaaaaaaaa!!
Sign in to reply to this message.
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)
Sign in to reply to this message.
2011/6/15 <percival.music.ca@gmail.com>: > 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
Sign in to reply to this message.
"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.
Sign in to reply to this message.
W dniu 15 czerwca 2011 00:49 użytkownik Carl Sorensen <c_sorensen@byu.edu> napisał: > > On 6/14/11 4:19 PM, "Janek Warchoł" <lemniskata.bernoullego@gmail.com> > 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 <karin.hoethker@googlemail.com>: > > "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
Sign in to reply to this message.
On Wed, Jun 15, 2011 at 01:02:07PM +0200, Janek Warchoł wrote: > 2011/6/15 <percival.music.ca@gmail.com>: > > 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
Sign in to reply to this message.
2011/6/15 Graham Percival <graham@percival-music.ca>: > On Wed, Jun 15, 2011 at 01:02:07PM +0200, Janek Warchoł wrote: >> 2011/6/15 <percival.music.ca@gmail.com>: >> > 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 "aaaaaaaaaaaaaaaaaaa!! 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
Sign in to reply to this message.
On Wed, Jun 15, 2011 at 02:05:10PM +0200, Janek Warchoł wrote: > Patches attached. Rebased from 12 to 5 commits; i kept > "aaaaaaaaaaaaaaaaaaa!! 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
Sign in to reply to this message.
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.
Sign in to reply to this message.
2011/6/15 <percival.music.ca@gmail.com>: > 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
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
