Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1)

Issue 5729051: Uses derived_mark to avoid segfault (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years ago by MikeSol
Modified:
12 years ago
Reviewers:
Keith, dak, Neil Puttock
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Uses 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -15 lines) Patch
M lily/span-bar-stub-engraver.cc View 1 2 3 9 chunks +62 lines, -15 lines 7 comments Download

Messages

Total messages: 9
MikeSol
Hey all, I don't have time to test this patch tonight (David sent me to ...
12 years ago (2012-03-03 22:27:16 UTC) #1
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 ...
12 years ago (2012-03-04 07:26:29 UTC) #2
MikeSol
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 ...
12 years ago (2012-03-04 08:44:06 UTC) #3
Neil 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 ...
12 years ago (2012-03-04 10:47:17 UTC) #4
Keith
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, ...
12 years ago (2012-03-04 20:59:22 UTC) #5
MikeSol
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 > ...
12 years ago (2012-03-04 21:12:30 UTC) #6
Keith
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 ...
12 years ago (2012-03-04 21:35:57 UTC) #7
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 ...
12 years ago (2012-03-06 12:19:04 UTC) #8
MikeSol
12 years ago (2012-03-07 07:23:28 UTC) #9
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b