Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden)

[v . villenave]

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#newcode116
> 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.

https://codereview.appspot.com/576090043/