Re: Uses derived_mark to avoid segfault (issue 5729051)

2012-03-06 Thread dak 


http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode93
lily/span-bar-stub-engraver.cc:93: if (!scm_is_pair (axis_groups_) ||
!scm_is_pair (scm_car (axis_groups_)))
I'd remove the second check here.  The first check makes sure that we
have got data to operate on (so it checks the overall algorithmic
consistency), but the second check just checks that the data is of the
form we actually put in here, and since it is private data, nobody could
have changed it but ourselves.  It is a test of the
if (1 == 0) cerr  Your compiler or CPU is broken;
variety.  Even if it triggered, the error message would not fit the
problem.

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode160
lily/span-bar-stub-engraver.cc:160: // removes all unused contexts
On 2012/03/04 10:47:17, Neil Puttock wrote:

Is it feasible to listen for RemoveContext instead?  At least then you

only need

to rebuild axis_groups_ when contexts die.


I'd second this suggestion.  It should give more reliable lifetimes, and
also cause a better correlation of parsed object should be dead
messages with realities.

http://codereview.appspot.com/5729051/

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

Re: Uses derived_mark to avoid segfault (issue 5729051)

2012-03-06 Thread mtsolo 


http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode35
lily/span-bar-stub-engraver.cc:35: these four contexts.
On 2012/03/04 21:35:57, Keith wrote:

You said later that only two SpanBarStubs are created, so
creation of SpanBarStubs will be contemplated in each of these four

contexts.

Fixed.

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode160
lily/span-bar-stub-engraver.cc:160: // removes all unused contexts
On 2012/03/06 12:19:04, dak wrote:

On 2012/03/04 10:47:17, Neil Puttock wrote:
 Is it feasible to listen for RemoveContext instead?  At least then

you only

need
 to rebuild axis_groups_ when contexts die.



I'd second this suggestion.  It should give more reliable lifetimes,

and also

cause a better correlation of parsed object should be dead messages

with

realities.


Unfortunately this is not possible.  I tried, but it looks like
listeners in the way you're talking about are not possible in engravers.

http://codereview.appspot.com/5729051/

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

Re: Uses derived_mark to avoid segfault (issue 5729051)

2012-03-04 Thread mtsolo 


http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode71
lily/span-bar-stub-engraver.cc:71: axis_groups_ = scm_cons (scm_cons
(i.grob ()-self_scm (), i.context ()-self_scm ()), axis_groups_);
On 2012/03/04 07:26:30, dak wrote:

axis_groups_ never gets cleared out again: this keeps grobs and

contexts alive

at least until the engraver is collected.


This is the point.  I was wrong to suggest before that it be cleared
out.
Unless the score is insane, a context like PianoStaff will only ever
house 10ish contexts and 10 vertical axis groups max.  So the size will
stay tiny.

I could clear this out, but the problem is that LilyPond doesn't provide
a mechanism to signal when VerticalAxisGroups are no longer being used.
If announce_end_spanner were called on them, I could acknowledge that in
the engraver.  A separate tracker issue could be opened for that.

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode81
lily/span-bar-stub-engraver.cc:81: if (!vertical_alignment) // we are at
the beginning of a score, so no need for stubs
On 2012/03/04 07:26:30, dak wrote:

If we are at the beginning of the score is a valid state here,

scm_caar

(axis_groups_) is likely to throw an exception.


This is not possible because process_acknowledged is called after all
other process calls, so the list will be populated.  However, I agree
that an extra check here couldn't hurt.

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode127
lily/span-bar-stub-engraver.cc:127: for (vsize j = 0; j 
affected_contexts.size (); j++)
On 2012/03/04 07:26:30, dak wrote:

While you are at it: how about throwing out all of the reverses and

instead

letting the loop run backwards?


This is vestigial code from when I was doing sorting on the span bar
vector.  It is no longer necessary, so removed.

http://codereview.appspot.com/5729051/

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

Re: Uses derived_mark to avoid segfault (issue 5729051)

2012-03-04 Thread n . puttock 


http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode160
lily/span-bar-stub-engraver.cc:160: // removes all unused contexts
Is it feasible to listen for RemoveContext instead?  At least then you
only need to rebuild axis_groups_ when contexts die.

http://codereview.appspot.com/5729051/

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

Re: Uses derived_mark to avoid segfault (issue 5729051)

2012-03-04 Thread mtsolo 

On 2012/03/04 20:59:22, Keith wrote:

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc

File lily/span-bar-stub-engraver.cc (right):



http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode46

lily/span-bar-stub-engraver.cc:46: be engraved in contexts where

BarLines are

engraved.
So, in the example with the piano staff earlier in the comment,
Four SpanBarStubs are /created/ at each barline,
Two are /engraved/ (those in the Lyrics and Dynamics context which

have the

Pure_from_neightbor_engraver, but not those in the Staffs which have

the

Bar_line_engraver) and
Zero are printed.



Right?


Close.
Two are created, two are engraved, and 0 are printed.  The
Pure_from_neighbor_engraver would catch them in Staves too, but they are
not sent to staves or anything else that has BarLines, as BarLines
fulfill the role of SpanBarStubs in contexts that have them.  The line
that makes sure that contexts with barlines do not get SpanBarStubs is:

find (bar_axis_indices.begin (), bar_axis_indices.end (), j) ==
bar_axis_indices.end ()

Or, in other words, only create the grob (and thus do the engraving) if
the context's index is not in the list of indices of contexts that have
bar lines (== bar_axis_indices.end () is another way of saying that it
is not in the vector, so find returns a pointer to the vector's end).

http://codereview.appspot.com/5729051/

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

Re: Uses derived_mark to avoid segfault (issue 5729051)

2012-03-04 Thread k-ohara5a5a 


http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode35
lily/span-bar-stub-engraver.cc:35: these four contexts.
You said later that only two SpanBarStubs are created, so
creation of SpanBarStubs will be contemplated in each of these four
contexts.

http://codereview.appspot.com/5729051/

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

Uses derived_mark to avoid segfault (issue 5729051)

2012-03-03 Thread mtsolo 

Reviewers: ,

Message:
Hey all,

I don't have time to test this patch tonight (David sent me to bed) but
it should do the trick.  The map was overkill - a simple list of pairs
does the trick w/o anymore overhead and allows the use of derived_mark.

Cheers,
MS

Description:
Uses derived_mark to avoid segfault

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

Affected files:
  M lily/span-bar-stub-engraver.cc


Index: lily/span-bar-stub-engraver.cc
diff --git a/lily/span-bar-stub-engraver.cc b/lily/span-bar-stub-engraver.cc
index  
7b26ddbd1e68ed1d0892471d3f465650661e1bdf..35a47207fa5f353519889393ca1a8e948883ac2f  
100644

--- a/lily/span-bar-stub-engraver.cc
+++ b/lily/span-bar-stub-engraver.cc
@@ -17,7 +17,6 @@
   along with LilyPond.  If not, see http://www.gnu.org/licenses/.
 */

-#include map
 #include algorithm

 #include align-interface.hh
@@ -38,7 +37,7 @@
 class Span_bar_stub_engraver : public Engraver
 {
   vectorGrob * spanbars_;
-  mapGrob *, Context * axis_groups_;
+  SCM axis_groups_;

 public:
   TRANSLATOR_DECLARATIONS (Span_bar_stub_engraver);
@@ -46,10 +45,18 @@ protected:
   DECLARE_ACKNOWLEDGER (span_bar);
   DECLARE_ACKNOWLEDGER (hara_kiri_group_spanner);
   void process_acknowledged ();
+  virtual void derived_mark () const;
 };

 Span_bar_stub_engraver::Span_bar_stub_engraver ()
 {
+  axis_groups_ = SCM_EOL;
+}
+
+void
+Span_bar_stub_engraver::derived_mark () const
+{
+  scm_gc_mark (axis_groups_);
 }

 void
@@ -61,7 +68,7 @@ Span_bar_stub_engraver::acknowledge_span_bar (Grob_info i)
 void
 Span_bar_stub_engraver::acknowledge_hara_kiri_group_spanner (Grob_info i)
 {
-  axis_groups_[i.grob ()] = i.context ();
+  axis_groups_ = scm_cons (scm_cons (i.grob ()-self_scm (), i.context  
()-self_scm ()), axis_groups_);

 }

 void
@@ -70,12 +77,10 @@ Span_bar_stub_engraver::process_acknowledged ()
   if (!spanbars_.size ())
 return;

-  Grob *vertical_alignment = Grob::get_root_vertical_alignment  
((*axis_groups_.begin ()).first);
+  Grob *vertical_alignment = Grob::get_root_vertical_alignment  
(unsmob_grob (scm_caar (axis_groups_)));
   if (!vertical_alignment) // we are at the beginning of a score, so no  
need for stubs

 return;

-  extract_grob_set (vertical_alignment, elements, elts);
-
   for (vsize i = 0; i  spanbars_.size (); i++)
 {
   extract_grob_set (spanbars_[i], elements, bars);
@@ -89,8 +94,14 @@ Span_bar_stub_engraver::process_acknowledged ()
   vectorContext * affected_contexts;
   vectorGrob * y_parents;
   vectorbool keep_extent;
-  for (vsize j = 0; j  elts.size (); j++)
+  for (SCM s = axis_groups_; scm_is_pair (s); s = scm_cdr (s))
 {
+  Context *c = unsmob_context (scm_cdar (s));
+  Grob *g = unsmob_grob (scm_caar (s));
+  if (!c || !g)
+continue;
+
+  vsize j = Grob::get_vertical_axis_group_index (g);
   if (j  bar_axis_indices[0]
j  bar_axis_indices.back ()
find (bar_axis_indices.begin (), bar_axis_indices.end (),  
j) == bar_axis_indices.end ())

@@ -101,12 +112,11 @@ Span_bar_stub_engraver::process_acknowledged ()
   break;

   k--;
-  keep_extent.push_back (to_boolean (bars[k]-get_property  
(allow-span-bar)));


-  Context *c = axis_groups_[elts[j]];
   if (c  c-get_parent_context ())
 {
-  y_parents.push_back (elts[j]);
+  keep_extent.push_back (to_boolean (bars[k]-get_property  
(allow-span-bar)));

+  y_parents.push_back (g);
   affected_contexts.push_back (c);
 }
 }
@@ -122,7 +132,7 @@ Span_bar_stub_engraver::process_acknowledged ()
   gi.rerouting_daddy_context_ = affected_contexts[j];
   announce_grob (gi);
   if (!keep_extent[j])
-it-suicide ();//it-set_property (Y-extent, ly_interval2scm  
(Interval (infinity_f, -infinity_f)));

+it-suicide ();
 }
 }
   spanbars_.clear ();



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

Re: Uses derived_mark to avoid segfault (issue 5729051)

2012-03-03 Thread dak 


http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode71
lily/span-bar-stub-engraver.cc:71: axis_groups_ = scm_cons (scm_cons
(i.grob ()-self_scm (), i.context ()-self_scm ()), axis_groups_);
axis_groups_ never gets cleared out again: this keeps grobs and contexts
alive at least until the engraver is collected.

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode81
lily/span-bar-stub-engraver.cc:81: if (!vertical_alignment) // we are at
the beginning of a score, so no need for stubs
If we are at the beginning of the score is a valid state here,
scm_caar (axis_groups_) is likely to throw an exception.

http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode127
lily/span-bar-stub-engraver.cc:127: for (vsize j = 0; j 
affected_contexts.size (); j++)
While you are at it: how about throwing out all of the reverses and
instead letting the loop run backwards?

http://codereview.appspot.com/5729051/

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