Checks to see if tuplet brackets have bounds

Checks to see if tuplet brackets have bounds

[dak, 2013-09-13 06:41:00]

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.

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.

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.

[mike7, 2013-09-13 07:09:44]

On 13 sept. 2013, at 08:41, dak@gnu.org 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/

[MikeSol, 2013-09-13 07:11:05]

Response to David's comments

[Trevor Daniels, 2013-09-13 08:41:42]

On 2013/09/13 07:09:44, mike7 wrote:

> 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)?

LilyPond should always try to continue when an error is detected.
If the error is thought to be a user error a warning should be
issued and an assumption made about what was intended.  If the
error is thought to be due to faulty or missing code (as here) the
erroneous operation should be abandoned and a programming error
issued.  Reported programming errors should always be recorded in
an issue tracker unless one already exists.  At least that, I 
believe, was the general approach adopted in the past and it seems
sound to me.

Trevor

[dak, 2013-09-13 09:00:35]

On 2013/09/13 08:41:42, Trevor Daniels wrote:
> On 2013/09/13 07:09:44, mike7 wrote:
> 
> > 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)?
> 
> LilyPond should always try to continue when an error is detected.
> If the error is thought to be a user error a warning should be
> issued and an assumption made about what was intended.  If the
> error is thought to be due to faulty or missing code (as here) the
> erroneous operation should be abandoned and a programming error
> issued.  Reported programming errors should always be recorded in
> an issue tracker unless one already exists.  At least that, I 
> believe, was the general approach adopted in the past and it seems
> sound to me.

Well, being defensive is always a good idea.  It's just that when we get to
_know_ the defenses to be triggered without us expecting it, we should make use
of the opportunity to analyze what is going wrong here.

Mike says that the change is due to more crossstaff checks happening now.  I
have no better guess here, but the issue description of issue 3199 does not at
all mention crossstaff, and the error does not occur in a context or in code
mentioning crossstaff.  That presumably means that the code and/or the issue is
factored lousily, resulting in problems triggered in/by mostly unrelated code
and circumstances.  And while we are adding additional safety checks, we better
think about what we can do to improve this situation.  Because afterwards, the
incentive for thinking about it will have disappeared.

The main question we need to ask ourselves is: does the condition for which we
do an early return here occur in circumstances where an early return is a
_correct_ answer?  If not, we should be raising a programming error here unless
the situation can _only_ occur in circumstances for which we already get a
programming error (but then we should not be calling the function in the first
place, should we?).

[mike7, 2013-09-13 10:10:57]

On 13 sept. 2013, at 11:00, dak@gnu.org wrote:

> On 2013/09/13 08:41:42, Trevor Daniels wrote:
>> On 2013/09/13 07:09:44, mike7 wrote:
> 
>> > 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)?
> 
>> LilyPond should always try to continue when an error is detected.
>> If the error is thought to be a user error a warning should be
>> issued and an assumption made about what was intended.  If the
>> error is thought to be due to faulty or missing code (as here) the
>> erroneous operation should be abandoned and a programming error
>> issued.  Reported programming errors should always be recorded in
>> an issue tracker unless one already exists.  At least that, I
>> believe, was the general approach adopted in the past and it seems
>> sound to me.
> 
> Well, being defensive is always a good idea.  It's just that when we get
> to _know_ the defenses to be triggered without us expecting it, we
> should make use of the opportunity to analyze what is going wrong here.
> 
> Mike says that the change is due to more crossstaff checks happening
> now.  I have no better guess here, but the issue description of issue
> 3199 does not at all mention crossstaff, and the error does not occur in
> a context or in code mentioning crossstaff.

To be more specific, when the pure height of axis groups is being calculated, it
iterates over all pure relevant grobs to find their heights, throwing out cross
staff grobs because their heights cannot be calculated because they are by
definition not part of the axis group.

Tuplet brackets used not to be tagged pure relevant because we had those huge
lists of what was and wasn't.  Now, pure relevant is a much larger list of grobs
because pure relationships are articulated via unpure-pure-containers and not
lists.  So, we iterate over a larger set of grobs.  What this means is that
certain grobs that never had their cross-staffitude checked now do.

I believe that the correct approach is to make sure that every grob has a
cross-staff function that can be called at _any_ time after engraving is done. 
Here, perhaps we should add extra checks before parallel beam is called to weed
out boundless spanners.

Cheers,
MS

[MikeSol, 2013-09-13 10:17:18]

Adds check in cross-staff function before programming error

[Keith, 2013-09-13 20:12:35]

The first two patches look good.

Defensive checks for null pointers are a good thing.
We do not want a bracket for the input in the bug-report. 
We want a message when our .ly input asks for a tuplet bracket that cannot be
printed, and we get such a message (just before a crash, without this patch).

Maybe "warning" would be more appropriate than "programming error" as the
message in tuplet-engraver.cc, since LilyPond allows both the input in the bug
report and this variant where we want, and get, a bracket:
  \new Voice << {
    %%% \once\override Voice.TupletBracket #'cross-staff = ##f
    \times 2/3 { s8 s8 s8 } r2. }
    { b8*2/3 b b } >>

If there are no bounds to attach the bracket to, it would make sense to skip the
creation of the TupletBracket with a continue; statement after the error message
in tuplet-engraver.cc

https://codereview.appspot.com/13582046/diff/11001/lily/tuplet-bracket.cc
File lily/tuplet-bracket.cc (right):

https://codereview.appspot.com/13582046/diff/11001/lily/tuplet-bracket.cc#new...
lily/tuplet-bracket.cc:809: 
Why check the bounds here? 
We certainly don't want to check break_status_dir() here.

Conceptually, the (line-broken) tuplet /could/ end on a line-break, but still be
cross-staff.
In fact, the first time cross-staff is called is now before line-breaking, so
break_status_dir() is 0, and the value of cross-staff is stored in the property
so it need not be called again.

Now that there is no more pure-relevant filter, adjacent_pure_heights() should
probably use the get_pure_property() method of checking cross-staff, omitting
object for which cross-staff is true or non-boolean (a.k.a. "don't know yet").