Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl Sorensen]

On 7/10/11 4:41 PM, "Neil Puttock"  wrote:

> On 10 July 2011 23:34, Carl Sorensen  wrote:
> 
>> Ahh, thanks.  Fixed now:
>> 
>> {
>>  \new Voice \with  {
>>    \consists Ambitus_engraver
>>    \consists Mensural_ligature_engraver
> 
> Heh, you didn't have to do this; I meant the missing quotation marks
> around the engraver names. :)

Ahh -- oh, well.  The new version *is* shorter.
> 
> (and now you've added a spurious sequential block ;)

And removed it.

Thanks,

Carl


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Neil Puttock]

On 10 July 2011 23:34, Carl Sorensen  wrote:

> Ahh, thanks.  Fixed now:
>
> {
>  \new Voice \with  {
>    \consists Ambitus_engraver
>    \consists Mensural_ligature_engraver

Heh, you didn't have to do this; I meant the missing quotation marks
around the engraver names. :)

(and now you've added a spurious sequential block ;)

Cheers,
Neil

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl Sorensen]

On 7/10/11 3:09 PM, "Neil Puttock"  wrote:

> On 7 July 2011 23:22,   wrote:
>> On 2011/07/07 16:12:08, Neil Puttock wrote:
>>> 
>>> LGTM, though the regtest is still a bit messy.
>> 
>> What do you consider messy about the regtest?
> 
> Just a few nitpicks:
> 
> +\score{
> 
> + \context Staff="default" {
> 
> + \context{
> 
> + \consists Ambitus_engraver
> + \consists Mensural_ligature_engraver

Ahh, thanks.  Fixed now:

{
  \new Voice \with  {
\consists Ambitus_engraver
\consists Mensural_ligature_engraver
} {
  \[ c'\longa c''\longa \]
}
}

Thanks,

Carl


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Neil Puttock]

On 7 July 2011 23:22,   wrote:
> On 2011/07/07 16:12:08, Neil Puttock wrote:
>>
>> LGTM, though the regtest is still a bit messy.
>
> What do you consider messy about the regtest?

Just a few nitpicks:

+\score{

+ \context Staff="default" {

+ \context{

+ \consists Ambitus_engraver
+ \consists Mensural_ligature_engraver

Cheers,
Neil

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl Sorensen]

On 7/9/11 7:55 PM, "colinpkcampb...@gmail.com" 
wrote:

> This has had a 48-hour countdown, and can be pushed and closed, please.

Issue 1715 issue marked fixed; Rietveld issue closed

Commit: 4fd0de625eba203265241fca20c504724013f534

Thanks,

Carl


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[ColinPKCampbell]

This has had a 48-hour countdown, and can be pushed and closed, please.

Colin

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl . D . Sorensen]

On 2011/07/07 16:12:08, Neil Puttock wrote:

LGTM, though the regtest is still a bit messy.


What do you consider messy about the regtest?



scm/define-grob-interfaces.scm:128: "A note-head that can become part

of a

ligature."
note head


Done

Thanks,

Carl




http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[n . puttock]

LGTM, though the regtest is still a bit messy.


http://codereview.appspot.com/4667055/diff/14001/scm/define-grob-interfaces.scm
File scm/define-grob-interfaces.scm (right):

http://codereview.appspot.com/4667055/diff/14001/scm/define-grob-interfaces.scm#newcode128
scm/define-grob-interfaces.scm:128: "A note-head that can become part of
a ligature."
note head

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl . D . Sorensen]

On 2011/07/05 19:58:38, Neil Puttock wrote:

Erm, can I take back what I said yesterday? :)



This looks really weird now, since it appears we're acknowledging

ligature

grobs.  I think ligature-head-interface would be less confusing for

the

acknowledgers.


Not only did it look really weird, it was wrong, even though I don't
know what the code should be to prove the error, but I think it would be
part of gregorian engraving, where we create ligatures.

ligature_engraver *listens* to ligatures, and *acknowledges*
ligature_heads.

I've fixed it properly now,.

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[n . puttock]

On 2011/07/04 22:31:18, Carl wrote:


OK, so I've added ligature-interface to NoteHead, and everything seems

to work

now.


Erm, can I take back what I said yesterday? :)

This looks really weird now, since it appears we're acknowledging
ligature grobs.  I think ligature-head-interface would be less confusing
for the acknowledgers.

Cheers,
Neil

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl . D . Sorensen]

On 2011/07/04 21:22:58, Neil Puttock wrote:

On 4 July 2011 21:57,   wrote:



> We already have the unused ligature-interface.  What if I just

added

> ligature-interface to NoteHead?



Sounds fine to me.



OK, so I've added ligature-interface to NoteHead, and everything seems
to work now.

Thanks,
Carl

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Neil Puttock]

On 4 July 2011 21:57,   wrote:

> The original way you suggested was to have two internal_has_interface ()
> calls; this one only adds one.

But for the invalid heads, all three calls would be made (and of
course, for a NoteHead itself only the first interface check will ever
happen, so the other calls are redundant).

> We already have the unused ligature-interface.  What if I just added
> ligature-interface to NoteHead?

Sounds fine to me.

Cheers,
Neil

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl . D . Sorensen]

On 2011/07/04 20:30:10, Neil Puttock wrote:

On 2011/07/04 01:54:40, Carl wrote:
> Instead of filtering out bad events, I chose to filter in only

events with

> ligature interfaces.



That's a lot of internal_has_interface () calls. :)


I wondered about that.  But I think the first one that is true will stop
the evaluation, so at the present only one would be called.

The original way you suggested was to have two internal_has_interface ()
calls; this one only adds one.



You might as well create a
new interface just for NoteHead (e.g., ligature-head-interface).



We already have the unused ligature-interface.  What if I just added
ligature-interface to NoteHead?

Thanks,

Carl


http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[n . puttock]

On 2011/07/04 01:54:40, Carl wrote:

Instead of filtering out bad events, I chose to filter in only events

with

ligature interfaces.


That's a lot of internal_has_interface () calls. :)  You might as well
create a new interface just for NoteHead (e.g.,
ligature-head-interface).

Cheers,
Neil

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl . D . Sorensen]

Instead of filtering out bad events, I chose to filter in only events
with ligature interfaces.

I'm not sure my line-breaking is correct on the internal_has_interface
() calls, but we'll soon have fixcc.py working, right? :)  If it isn't
right, please help me know how to fix it.

I also trimmed the regtest.

Thanks,

Carl



http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc
File lily/ligature-engraver.cc (right):

http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc#newcode199
lily/ligature-engraver.cc:199: if (info.event_cause ()) // Ignore
AmbitusNoteHead which has no event_cause
On 2011/07/03 21:25:58, Neil Puttock wrote:

if (info.event_cause ()
 && ligature_)


Done.

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[pkx166h]

Passes make and reg test

James

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[n . puttock]

On 2011/07/03 21:39:16, Neil Puttock wrote:

On 2011/07/03 21:32:45, Carl wrote:
> On 2011/07/03 21:25:58, Neil Puttock wrote:
> > Hi Carl,
> >
> > This LGTM, though the regtest could do with a bit of tidying.
>
> Yeah, I thought of that, but I was too lazy.  I just took what was

in the

> tracker.
>
> OK, I'll trim it up some more.
>
> >
> > A more correct approach would be to filter interfaces since the
> > Ligature_engraver also potentially acknowledges PitchTrillGroup.
>
> What do you mean "filter interfaces"?  As long as I'm working on it

I might as

> well get it right.



Use info.grob ()->internal_has_interface ("...") to filter out the
AmbitusNoteHead and PitchTrillGroup (via ambitus-interface and
axis-group-interface).


Oops, TrillPitchGroup.

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[n . puttock]

On 2011/07/03 21:32:45, Carl wrote:

On 2011/07/03 21:25:58, Neil Puttock wrote:
> Hi Carl,
>
> This LGTM, though the regtest could do with a bit of tidying.



Yeah, I thought of that, but I was too lazy.  I just took what was in

the

tracker.



OK, I'll trim it up some more.



>
> A more correct approach would be to filter interfaces since the
> Ligature_engraver also potentially acknowledges PitchTrillGroup.



What do you mean "filter interfaces"?  As long as I'm working on it I

might as

well get it right.


Use info.grob ()->internal_has_interface ("...") to filter out the
AmbitusNoteHead and PitchTrillGroup (via ambitus-interface and
axis-group-interface).

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl . D . Sorensen]

On 2011/07/03 21:25:58, Neil Puttock wrote:

Hi Carl,



This LGTM, though the regtest could do with a bit of tidying.


Yeah, I thought of that, but I was too lazy.  I just took what was in
the tracker.

OK, I'll trim it up some more.



A more correct approach would be to filter interfaces since the
Ligature_engraver also potentially acknowledges PitchTrillGroup.


What do you mean "filter interfaces"?  As long as I'm working on it I
might as well get it right.

Thanks,

Carl


http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[n . puttock]

Hi Carl,

This LGTM, though the regtest could do with a bit of tidying.

A more correct approach would be to filter interfaces since the
Ligature_engraver also potentially acknowledges PitchTrillGroup.  It's
probably not worth the bother though: it does have an event-cause and is
ignored by the ligature engravers; furthermore, it's unlikely
\pitchedTrill would be used in conjunction with such ligatures.

Cheers,
Neil


http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc
File lily/ligature-engraver.cc (right):

http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc#newcode199
lily/ligature-engraver.cc:199: if (info.event_cause ()) // Ignore
AmbitusNoteHead which has no event_cause
if (info.event_cause ()
&& ligature_)

http://codereview.appspot.com/4667055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix segfault with ambitus and ligature (Issue 1715) (issue4667055)

[Carl . D . Sorensen]

Reviewers: ,

Message:
Here is a proposed patch for fixing issue 1715.  It works by checking
for event-cause before acknowledging a notehead, thus ignoring
AmbitusNoteHeads

Description:
Fix segfault with ambitus and ligature (Issue 1715)
Check for an event-cause before acknowledging note_head.  This
prevents trying to make a ligature from the AmbitusNoteHeads.

Also includes regression test.

Please review this at http://codereview.appspot.com/4667055/

Affected files:
  A input/regression/ambitus-with-ligature.ly
  M lily/ligature-engraver.cc


Index: input/regression/ambitus-with-ligature.ly
diff --git a/input/regression/ambitus-with-ligature.ly  
b/input/regression/ambitus-with-ligature.ly

new file mode 100644
index  
..49c1f27ab7d3afa0caa2693fd8f6fef5f3c2eb79

--- /dev/null
+++ b/input/regression/ambitus-with-ligature.ly
@@ -0,0 +1,29 @@
+\version "2.14"
+
+\header {
+  texidoc = "
+A @code{\Voice} should be able to contain both an @code{Ambitus_engraver}
+and a @code{Mensural_ligature_engraver} without segfaulting.
+  "
+}
+
+testnotes = {
+  \relative c' {
+\[ c\longa c'\longa \] %This is a ligature; we are interpreting it as  
two whole notes

+  }
+}
+
+\score{
+  {
+\context Staff="default" {
+  \testnotes
+}
+  }
+  \layout {
+\context{
+  \Voice
+  \consists Ambitus_engraver
+  \consists Mensural_ligature_engraver
+}
+  }
+}
Index: lily/ligature-engraver.cc
diff --git a/lily/ligature-engraver.cc b/lily/ligature-engraver.cc
index  
be4917f0c735ec5da9078c875af32a0317ce35ea..756b7bbc7d44569ff873d77c5f89e1d45d7092ae  
100644

--- a/lily/ligature-engraver.cc
+++ b/lily/ligature-engraver.cc
@@ -143,7 +143,7 @@ Ligature_engraver::process_music ()

   ligature_start_mom_ = now_mom ();

-  // TODO: dump cause into make_item/spanner.
+  // TODO: dump cause into make_item/spanner.
   // announce_grob (ligature_, events_drul_[START]->self_scm ());
 }
 }
@@ -196,14 +196,13 @@ Ligature_engraver::current_ligature ()
 void
 Ligature_engraver::acknowledge_note_head (Grob_info info)
 {
-  if (ligature_)
-{
-  primitives_.push_back (info);
-  if (info.grob () && brew_ligature_primitive_proc != SCM_EOL)
-   {
+  if (info.event_cause ())
+if (ligature_)
+  {
+primitives_.push_back (info);
+if (info.grob () && brew_ligature_primitive_proc != SCM_EOL)
  info.grob ()->set_property ("stencil", brew_ligature_primitive_proc);
-   }
-}
+  }
 }

 void



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel