[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol.
From: |
v . villenave |
Subject: |
Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden) |
Date: |
Sat, 09 May 2020 11:14:40 -0700 |
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/
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), nine . fierce . ballads, 2020/05/07
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), v . villenave, 2020/05/07
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), Carl . D . Sorensen, 2020/05/07
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), nine . fierce . ballads, 2020/05/08
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), nine . fierce . ballads, 2020/05/08
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), v . villenave, 2020/05/09
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), nine . fierce . ballads, 2020/05/09
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden),
v . villenave <=
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), nine . fierce . ballads, 2020/05/09
- Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by address@hidden), v . villenave, 2020/05/09