Re: Checks to see if tuplet brackets have bounds (issue 13582046)

[Mike Solomon]

On 13 sept. 2013, at 08:41, address@hidden wrote:

> 
> https://codereview.appspot.com/13582046/diff/1/lily/tuplet-bracket.cc
> File lily/tuplet-bracket.cc (right):
> 
> https://codereview.appspot.com/13582046/diff/1/lily/tuplet-bracket.cc#newcode99
> lily/tuplet-bracket.cc:99: if (!left || !right)
> Too many combinatorics involved unnecessarily.  Just do
> if (!left || left->break_status_dir ())
> first, and then the same for right.  Then one does not need to track the
> combination of left/right states at the same time in order to figure out
> which cases are caught.

Ok

> 
> At any rate, this does not appear to give us a beam: it just avoids a
> crash.  And there is no explanation of why this code would be called now
> when it wasn't called before issue 3199.

We check cross staff for a lot more things now.

> 
> Not sure whether one would call this related to issue 3299: issue 3299
> is for the case where _no_ other rhythmic elements share the time of the
> spacer rest.  Which is the case in the minimal example of the bug report
> but possibly not the only situation triggering the bug.
> 
> At any rate, this appears to be a symptomatic patch replacing a crash
> with a programming error and/or a bad result.  Which is an improvement
> but still seems to fall short of useful behavior.

In general, there is a lot of old code in the code base that assumes a valid 
pointer will be returned and uses that pointer to get information without 
checking if it is a null pointer.  This is an example of that.  The reason it 
was never triggered before is because the cross-staffitude of these grobs was 
not checked before.

Here's the way I see the history:
-) We create a tuplet bracket engraver that can create a tuplet bracket without 
bounds.
-) We create function that checks these bounds (parallel_beam).
-) Before 2.17, this function is not called when there are no bounds.

To me, the bug is not the calling of the function but rather creating a 
behavior in this (or any) function that assumes a non-null pointer will be 
returned.  There is always the possibility that that will happen.  I am guilty 
of coding this way as well, but I've tried to limit these types of assumptions 
and always check for null pointers.

With respect to your point about null pointers and the nature of the patch, I 
agree that there needs to be a better way to handle this.  To me, the general 
problem seems to be "what do we do when we assume a grob will have something 
(bound, object, etc.) and it doesn't?"  Is the solution to suicide the spanner 
if it doesn't have bounds?  Is the solution to raise a programming error like 
we do now (I prefer that solution)?

Cheers,
MS

> 
> https://codereview.appspot.com/13582046/