|
|
Created:
12 years ago by MikeSol Modified:
12 years ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionUses derived_mark to avoid segfault
Patch Set 1 #Patch Set 2 : Removes map include from span-bar-stub-engraver.cc #
Total comments: 6
Patch Set 3 : Fixes suggested by David. #Patch Set 4 : Adds explanations and weeds out unused contexts. #
Total comments: 7
MessagesTotal messages: 9
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
Sign in to reply to this message.
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.c... 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.c... 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.c... 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?
Sign in to reply to this message.
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.c... 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.c... 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.c... 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.
Sign in to reply to this message.
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.c... 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.
Sign in to reply to this message.
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.c... 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?
Sign in to reply to this message.
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.c... > 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).
Sign in to reply to this message.
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.c... 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."
Sign in to reply to this message.
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.c... 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.c... 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.
Sign in to reply to this message.
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.c... 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.c... 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.
Sign in to reply to this message.
|