Re: Uses derived_mark to avoid segfault (issue 5729051)
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)
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)
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
Re: Uses derived_mark to avoid segfault (issue 5729051)
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)
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? 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)
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)
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)
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