prevent collision of ligatures and next note

prevent collision of ligatures and next note

[benko.pal, 2012-10-19 19:25:05]

mensural-ligatures.ly is listed as changed - that is not surprising, although I
couldn't tell in advance _how_ it will change.  what is a bit more surprising
that I can't tell even now, having seen the regtest comparison.

[janek, 2012-10-23 13:18:12]

(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....
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?

[benko.pal, 2012-10-23 18:56:52]

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<Grob_info> 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....
> 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

[janek, 2012-10-24 09:33:28]

hi Pal,

On Tue, Oct 23, 2012 at 8:56 PM,  <benko.pal@gmail.com> wrote:
> in C++ there should be a good reason to pass complex structures like
> std::vector<Grob_info> 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

[benko.pal, 2012-10-24 10:36:52]

>> in C++ there should be a good reason to pass complex structures like
>> std::vector<Grob_info> 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.

[janek, 2012-10-24 10:41:07]

On Wed, Oct 24, 2012 at 12:36 PM, Benkő Pál <benko.pal@gmail.com> wrote:
>>> in C++ there should be a good reason to pass complex structures like
>>> std::vector<Grob_info> 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

[dak, 2012-10-24 10:51:54]

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.

[benko.pal, 2012-10-24 13:11:22]

>> > 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.