hairpin.cc: consider suicide before drawing stencil; issue 2583

hairpin.cc: consider suicide before drawing stencil; issue 2583

[Keith, 2012-08-10 09:15:19]

http://codereview.appspot.com/6449126/diff/1/lily/grob-property.cc
File lily/grob-property.cc (right):

http://codereview.appspot.com/6449126/diff/1/lily/grob-property.cc#newcode254
lily/grob-property.cc:254: *alist = scm_assq_remove_x (*alist, sym);
Separate fix.
Earlier we added an entry (sym . marker) to the associative list of properties,
where marker is 'calculation-in-progress, and we just confirmed that the entry
is still there.  Now that the calculation is finished, we want to  remove the
entry, but we need to index the list using sym, not marker.

[Keith, 2012-08-11 02:36:13]

http://codereview.appspot.com/6449126/diff/1003/input/regression/hairpin-barl...
File input/regression/hairpin-barline-break.ly (right):

http://codereview.appspot.com/6449126/diff/1003/input/regression/hairpin-barl...
input/regression/hairpin-barline-break.ly:11: line-width = 4.\cm
parallel with dynamics-broken-hairpin.ly

[dak, 2012-08-23 10:09:18]

http://codereview.appspot.com/6449126/diff/1003/lily/grob-property.cc
File lily/grob-property.cc (right):

http://codereview.appspot.com/6449126/diff/1003/lily/grob-property.cc#newcode254
lily/grob-property.cc:254: *alist = scm_assq_remove_x (*alist, sym);
I was considering cherry-picking this part of the change.  However, if the
comment above is supposed to make any sense, namely that the property is assumed
to be set (or cleared), then the assertion would have to be "assert (value !=
marker)", and the path "value == marker" should never be reached at all.

The whole situation seems so fishy that I won't change the status quo for
stable/2.16 for now.

[Keith, 2012-08-24 04:03:30]

http://codereview.appspot.com/6449126/diff/1003/lily/grob-property.cc
File lily/grob-property.cc (right):

http://codereview.appspot.com/6449126/diff/1003/lily/grob-property.cc#newcode254
lily/grob-property.cc:254: *alist = scm_assq_remove_x (*alist, sym);
On 2012/08/23 10:09:18, dak wrote:
> However, if the comment above is supposed to make any sense, 
> namely that the property is assumed
> to be set (or cleared) [by the callback]
> then the assertion would have to be "assert (value !=
> marker)", and the path "value == marker" should never be reached at all.
> 

That comment made sense when first written in 2005, explaining why we are
reading a property that just set (with the calculation-in-progress marker).  Its
assumption didn't last very long; the next commit ten hours later added the
(buggy) call to scm_assq_remove_x() attempting to clear that marker, in case the
callback forgot.  (In today's code I find no callbacks clearing their own
markers.)  The assert was added two years after that, largely preventing the
scenario described in the comment from happening in any useful way.

I'll delete the comment.