Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol.

Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol.

[Valentin Villenave, 2020-05-07 08:26:26]

Use `oneline’ when there’s either no staff OR a one-line staff

[Dan Eble, 2020-05-07 12:18:48]

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
File lily/multi-measure-rest.cc (right):

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_positions (staff).size() == 1;
If there is no staff symbol, then there isn't even one line. Are you sure you
don't want this?

    bool oneline = staff ? /*etc.*/ : false;

Incidentally, I am surprised to see the amount of work involved in determining
whether the staff has one line.  It's not the kind of thing one expects to
involve heap allocation.  That's not a problem with your change, though.

[Valentin Villenave, 2020-05-07 15:12:04]

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
File lily/multi-measure-rest.cc (right):

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_positions (staff).size() == 1;
On 2020/05/07 12:18:48, Dan Eble wrote:
> If there is no staff symbol, then there isn't even one line. Are you sure you
> don't want this?
>     bool oneline = staff ? /*etc.*/ : false;

No, I actually want oneline to be true . I did consider changing the variable
name to no_multiple_lines, because that’s what it’s really about. I also
considered adding a comment such as:
// If there is no StaffSymbol, print MMrests on one (invisible) line.

> Incidentally, I am surprised to see the amount of work involved in determining
> whether the staff has one line.  It's not the kind of thing one expects to
> involve heap allocation.

I suspect this sort-of mirrors the way you’d do it in Scheme:
(if (> (length
        (ly:grob-property
         (ly:grob-object grob 'staff-symbol)
        'line-positions))
      0)
 etc.

If you have a simpler syntax in mind, please share :-)

Cheers,
-- V.

[Carl, 2020-05-07 16:03:54]

Here's a thought.

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
File lily/multi-measure-rest.cc (right):

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_positions (staff).size() == 1;
On 2020/05/07 15:12:03, Valentin Villenave wrote:
> On 2020/05/07 12:18:48, Dan Eble wrote:
> > If there is no staff symbol, then there isn't even one line. Are you sure
you
> > don't want this?
> >     bool oneline = staff ? /*etc.*/ : false;
> 
> No, I actually want oneline to be true . I did consider changing the variable
> name to no_multiple_lines, because that’s what it’s really about. I also
> considered adding a comment such as:
> // If there is no StaffSymbol, print MMrests on one (invisible) line.
> 
> > Incidentally, I am surprised to see the amount of work involved in
determining
> > whether the staff has one line.  It's not the kind of thing one expects to
> > involve heap allocation.
> 
> I suspect this sort-of mirrors the way you’d do it in Scheme:
> (if (> (length
>         (ly:grob-property
>          (ly:grob-object grob 'staff-symbol)
>         'line-positions))
>       0)
>  etc.
> 
> If you have a simpler syntax in mind, please share :-)
> 
> Cheers,
> -- V.

What about changing the name from oneline to specialcase?  Or maybe
default_position?  Then it would naturally apply to both no staff symbol and
one-line staff symbol.

[dan_faithful.be, 2020-05-07 21:41:28]

On May 7, 2020, at 11:12, v.villenave@gmail.com wrote:
> No, I actually want oneline to be true . I did consider changing the
> variable name to no_multiple_lines, because that’s what it’s really
> about. I also considered adding a comment such as:
> // If there is no StaffSymbol, print MMrests on one (invisible) line.

That comment would be sufficient.

> https://codereview.appspot.com/576090043/
— 
Dan

[Dan Eble, 2020-05-09 01:32:19]

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
File lily/multi-measure-rest.cc (right):

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_positions (staff).size() == 1;
On 2020/05/07 15:12:03, Valentin Villenave wrote:
> On 2020/05/07 12:18:48, Dan Eble wrote:
> > If there is no staff symbol, then there isn't even one line. Are you sure
you
...
> 
> No, I actually want oneline to be true .

Then to be consistent, shouldn't you also check for size() < 2 rather than
size() == 1?

[Dan Eble, 2020-05-09 01:42:54]

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
File lily/multi-measure-rest.cc (right):

https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-re...
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_positions (staff).size() == 1;
On 2020/05/07 15:12:03, Valentin Villenave wrote:
> On 2020/05/07 12:18:48, Dan Eble wrote:
> > Incidentally, I am surprised to see the amount of work involved in
determining
> > whether the staff has one line.  It's not the kind of thing one expects to
> > involve heap allocation.
...
> If you have a simpler syntax in mind, please share :-)

I looked around.  This is the only place I found using just the size of the
vector.  I don't think it's worth adding and maintaining a
Staff_symbol::line_count just for this case.

[Valentin Villenave, 2020-05-09 10:38:49]

[proposal] Make Staff_symbol::line_count public and use it when
<line_positions>’s not needed

[Valentin Villenave, 2020-05-09 10:43:25]

On 2020/05/09 01:42:54, Dan Eble wrote:
> I looked around.  This is the only place I found using just the size of the
> vector.  I don't think it's worth adding and maintaining a
> Staff_symbol::line_count just for this case.

Well, it wouldn’t be "adding and maintaining" since it already exists as an
internal, private function (and line_positions actually relies on it, thus using
that as a vector only adds an additional, unnecessary step).
What would be the cost of making it public, GC and performance-wise? (See my
latest patch set as a POC.)

Cheers,
-- V.

[Dan Eble, 2020-05-09 12:47:29]

https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc
File lily/staff-symbol.cc (right):

https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc#...
lily/staff-symbol.cc:116: return ly_scm2floatvector (line_positions);
The suggested patch to expose internal_line_count() as line_count() bypasses
this branch.  line_count() should have the same branches as line_positions() but
call scm_ilength() (I think) in this branch, and internal_line_count() in the
other branch.

Another thing is that it should return the type vsize, so you would need to take
care to clip the values you get from Guile to >= 0.

It isn't a lot of code, but it's the kind of thing that could easily get out of
sync with line_positions() when later contributors come by.  It's not something
I'd want to commit myself, but if you are interested in doing it, I'm willing to
say the L-word once it's ready.

[Valentin Villenave, 2020-05-09 18:14:40]

On 2020/05/09 12:47:29, Dan Eble wrote:
> https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc
> File lily/staff-symbol.cc (right):
> 
>
https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc#...
> lily/staff-symbol.cc:116: return ly_scm2floatvector (line_positions);
> The suggested patch to expose internal_line_count() as line_count() bypasses
> this branch.

Well, only in multi-measure-rest.cc (that’s the only place where only the line
count is used, not the full vector).

I do, however, worry about the comment before (what used to be) the
internal_line_count() definition:

// Get the line-count property directly.  This is for internal use when it is
// known that the line-positions property is not relevant.

Could there be any situations where the line-count SCM property and the
line_positions vector length would differ?

> line_count() should have the same branches as line_positions() but
> call scm_ilength() (I think) in this branch, and internal_line_count() in the
> other branch.

I don’t understand what you’re referring to.  The point of my latest patch is
that there’s no longer an internal_line_count(). The only (publicly exposed)
line_count() function that remains is inferred only from the line-count
property, as explained in the comment above.

The other point is that it now bypasses the vector’s creation altogether when
avoidable, whereas what you’re suggesting (IIUC, which I probably don’t) would
not.

> It isn't a lot of code, but it's the kind of thing that could easily get out
of
> sync with line_positions() when later contributors come by.

If I were to proceed like you’re suggesting, it may. But my proposal was much
simpler, as it just uses the existing internal_line_count() function. (Again, I
may not understand exactly what you’re suggesting, so please feel free to help
me out.)

> It's not something
> I'd want to commit myself, but if you are interested in doing it, I'm willing
to
> say the L-word once it's ready.

Heh. Didn’t they used to make a TV show about that? :-)

Cheers,
-- V.

[Dan Eble, 2020-05-09 18:30:04]

On 2020/05/09 18:14:40, Valentin Villenave wrote:
> I do, however, worry about the comment before (what used to be) the
> internal_line_count() definition:
> 
> // Get the line-count property directly.  This is for internal use when it is
> // known that the line-positions property is not relevant.
> 
> Could there be any situations where the line-count SCM property and the
> line_positions vector length would differ?

Yes, that's what I was trying to explain.  There are two different ways that
Staff_symbol::line_positions () can create a vector.  Therefore, to write a
correct replacement for ss.line_positions().size(), you need to predict the size
of the vector it would create under the same conditions.  internal_line_count()
is relevant to one branch only.

[Valentin Villenave, 2020-05-09 21:02:33]

Not worth messing around with staff-symbol functions. Reverting.

[Valentin Villenave, 2020-05-09 21:12:07]

On 2020/05/09 18:30:04, Dan Eble wrote:
> Yes, that's what I was trying to explain.
> internal_line_count() is relevant to one branch only.

Oh, I see your point now. I thought I could get away with line_count only, but
indeed this fails to account for (hypothetical) cases where the line-positions
property would be set but not the line-count property.  (I’m still not sure in
what sort of cases that conditional switch would ever be applicable, but it must
be there for a reason.)

Better not be messing around with that, then; the amount of simplification we’d
get would remain too marginal, and since it’s only used this once in
multi-measure-rest.cc, that’s certainly not worth it. I’m going back to my
original proposal.

Thanks for your guidance!

Cheers,
-- V.

[Dan Eble, 2020-05-09 21:59:10]

On 2020/05/09 21:12:07, Valentin Villenave wrote:
> Thanks for your guidance!

Glad to be of service.