Re: prevent collision of ligatures and next note (issue 6740046)
>> > Ah, so this is a by-the-way fix. Can it be in a separate commit, please? > >> it is; I meant to note it but forgot, sorry. is there interest in pushing >> such >> multi-commit patches to some dev branch? > > If there is, people should ask. Substructuring an issue into several > logical commits is good style and makes things easier afterwards, but > only few people will actively prefer reviewing changes in a branch over > using Rietveld, so you can save yourself the trouble of creating a > public branch unless someone asks for it. I meant opening a Rietveld issue with all changes and an announcement of a dev branch with several commits. > Just make sure that every commit leaves the tree in compilable state. of course. I've verified that the pass-by-reference commit didn't change the regtests. ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: prevent collision of ligatures and next note (issue 6740046)
On 2012/10/24 10:36:52, benko.pal wrote: [...] > Ah, so this is a by-the-way fix. Can it be in a separate commit, please? it is; I meant to note it but forgot, sorry. is there interest in pushing such multi-commit patches to some dev branch? If there is, people should ask. Substructuring an issue into several logical commits is good style and makes things easier afterwards, but only few people will actively prefer reviewing changes in a branch over using Rietveld, so you can save yourself the trouble of creating a public branch unless someone asks for it. Just make sure that every commit leaves the tree in compilable state. I've made branches available occasionally for humongous changes but I don't think they have been used much for reviewing. http://codereview.appspot.com/6740046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: prevent collision of ligatures and next note (issue 6740046)
On Wed, Oct 24, 2012 at 12:36 PM, Benkő Pál wrote: >>> in C++ there should be a good reason to pass complex structures like >>> std::vector by value, not by reference to const; in this case >>> there's no such reason, pass-by-reference works perfectly. >> >> Ah, so this is a by-the-way fix. Can it be in a separate commit, please? > > it is; I meant to note it but forgot, sorry. is there interest in pushing > such > multi-commit patches to some dev branch? sometimes, but i don't think this one is big enough to be worth the trouble. cheers, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: prevent collision of ligatures and next note (issue 6740046)
>> in C++ there should be a good reason to pass complex structures like >> std::vector by value, not by reference to const; in this case >> there's no such reason, pass-by-reference works perfectly. > > Ah, so this is a by-the-way fix. Can it be in a separate commit, please? it is; I meant to note it but forgot, sorry. is there interest in pushing such multi-commit patches to some dev branch? > As for the commit message, it you changed it, i don't see the new one. > You may want to try updating your git-cl with Julien Rioux's changes - > they improve description handling and other things. will look at it. ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: prevent collision of ligatures and next note (issue 6740046)
hi Pal, On Tue, Oct 23, 2012 at 8:56 PM, wrote: > in C++ there should be a good reason to pass complex structures like > std::vector by value, not by reference to const; in this case > there's no such reason, pass-by-reference works perfectly. Ah, so this is a by-the-way fix. Can it be in a separate commit, please? As for the commit message, it you changed it, i don't see the new one. You may want to try updating your git-cl with Julien Rioux's changes - they improve description handling and other things. > it _was_ related (see usage of dot_count lower); I hope the new > organization is cleaner. ah, i see it now, thanks. best, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: prevent collision of ligatures and next note (issue 6740046)
hi Janek, Pal, could you add some comments and a more descriptive commit message? I see the code, and it seems to make sense, but i don't understand the "why's". For example, why the parameters should be passed in a different way? in C++ there should be a good reason to pass complex structures like std::vector by value, not by reference to const; in this case there's no such reason, pass-by-reference works perfectly. I also think that it'd be a good idea to add a regtest, even if it's imperfect (just write a comment inside it, so that everyone envestigating it will be warned that some unexpected results may occur). the problem is that I really can't predict _when_ unexpected results can and when can't occur. I tried to come up with the Tiniest example and I'm sure my example in the attachment to http://code.google.com/p/lilypond/issues/detail?id=2914 is not the tiniest possible that demonstrates the problem. put otherwise, working all right with previous versions is just as unexpected for me as producing a collision. http://codereview.appspot.com/6740046/diff/1/lily/mensural-ligature-engraver.cc File lily/mensural-ligature-engraver.cc (right): http://codereview.appspot.com/6740046/diff/1/lily/mensural-ligature-engraver.cc#newcode428 lily/mensural-ligature-engraver.cc:428: if (size_t const dot_count = Rhythmic_head::dot_count (current)) Is this related to the ligature collision problem or just a by-the-way fix? it _was_ related (see usage of dot_count lower); I hope the new organization is cleaner. p http://codereview.appspot.com/6740046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: prevent collision of ligatures and next note (issue 6740046)
(as you may know, i dedicate all my code reviews to Graham the Magnificent :D) Pal, could you add some comments and a more descriptive commit message? I see the code, and it seems to make sense, but i don't understand the "why's". For example, why the parameters should be passed in a different way? I also think that it'd be a good idea to add a regtest, even if it's imperfect (just write a comment inside it, so that everyone envestigating it will be warned that some unexpected results may occur). cheers, Janek http://codereview.appspot.com/6740046/diff/1/lily/mensural-ligature-engraver.cc File lily/mensural-ligature-engraver.cc (right): http://codereview.appspot.com/6740046/diff/1/lily/mensural-ligature-engraver.cc#newcode428 lily/mensural-ligature-engraver.cc:428: if (size_t const dot_count = Rhythmic_head::dot_count (current)) Is this related to the ligature collision problem or just a by-the-way fix? http://codereview.appspot.com/6740046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
