Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1135)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by Valentin Villenave
Modified:
2 months ago
Reviewers:
Dan Eble, dan, carl.d.sorensen
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Use `oneline’ when there’s either no staff OR a one-line staff #

Total comments: 5

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

Total comments: 1

Patch Set 4 : Not worth messing around with staff-symbol functions. Reverting. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -2 lines) Patch
A input/regression/multi-measure-rest-no-staff.ly View 1 chunk +24 lines, -0 lines 0 comments Download
M lily/multi-measure-rest.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 15
Valentin Villenave
Use `oneline’ when there’s either no staff OR a one-line staff
2 months ago (2020-05-07 08:26:26 UTC) #1
Dan Eble
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268 lily/multi-measure-rest.cc:268: bool oneline = (!staff) || Staff_symbol::line_positions (staff).size() == 1; ...
2 months ago (2020-05-07 12:18:48 UTC) #2
Valentin Villenave
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268 lily/multi-measure-rest.cc:268: bool oneline = (!staff) || Staff_symbol::line_positions (staff).size() == 1; ...
2 months ago (2020-05-07 15:12:04 UTC) #3
Carl
Here's a thought. https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268 lily/multi-measure-rest.cc:268: bool oneline = (!staff) || Staff_symbol::line_positions ...
2 months ago (2020-05-07 16:03:54 UTC) #4
dan_faithful.be
On May 7, 2020, at 11:12, v.villenave@gmail.com wrote: > No, I actually want oneline to ...
2 months ago (2020-05-07 21:41:28 UTC) #5
Dan Eble
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268 lily/multi-measure-rest.cc:268: bool oneline = (!staff) || Staff_symbol::line_positions (staff).size() == 1; ...
2 months ago (2020-05-09 01:32:19 UTC) #6
Dan Eble
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268 lily/multi-measure-rest.cc:268: bool oneline = (!staff) || Staff_symbol::line_positions (staff).size() == 1; ...
2 months ago (2020-05-09 01:42:54 UTC) #7
Valentin Villenave
[proposal] Make Staff_symbol::line_count public and use it when <line_positions>’s not needed
2 months ago (2020-05-09 10:38:49 UTC) #8
Valentin Villenave
On 2020/05/09 01:42:54, Dan Eble wrote: > I looked around. This is the only place ...
2 months ago (2020-05-09 10:43:25 UTC) #9
Dan Eble
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() ...
2 months ago (2020-05-09 12:47:29 UTC) #10
Valentin 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 ...
2 months ago (2020-05-09 18:14:40 UTC) #11
Dan Eble
On 2020/05/09 18:14:40, Valentin Villenave wrote: > I do, however, worry about the comment before ...
2 months ago (2020-05-09 18:30:04 UTC) #12
Valentin Villenave
Not worth messing around with staff-symbol functions. Reverting.
2 months ago (2020-05-09 21:02:33 UTC) #13
Valentin Villenave
On 2020/05/09 18:30:04, Dan Eble wrote: > Yes, that's what I was trying to explain. ...
2 months ago (2020-05-09 21:12:07 UTC) #14
Dan Eble
2 months ago (2020-05-09 21:59:10 UTC) #15
On 2020/05/09 21:12:07, Valentin Villenave wrote:
> Thanks for your guidance!

Glad to be of service.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b