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

Issue 314490043: SCM/bool confusion in One_page_breaking::solve

Can't Edit
Can't Publish+Mail
Start Review
1 week, 2 days ago by dak
3 days, 22 hours ago


SCM/bool confusion in One_page_breaking::solve This replaces an accidentally always-true condition with the originally intended one. I have no idea about the consequences: it would be best if the original author Paul Morris (in issue 4752) checked that this now works as intended and also tried figuring out why we failed to notice this problem.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M lily/one-page-breaking.cc View 1 chunk +1 line, -1 line 1 comment Download


Total messages: 3
https://codereview.appspot.com/314490043/diff/1/lily/one-page-breaking.cc File lily/one-page-breaking.cc (right): https://codereview.appspot.com/314490043/diff/1/lily/one-page-breaking.cc#newcode97 lily/one-page-breaking.cc:97: if (scm_is_true (scm_gr_p (this_pos, lowest_line_pos))) Thanks for catching this. ...
5 days, 7 hours ago (2017-02-18 18:08:57 UTC) #1
On 2017/02/18 18:08:57, pwm wrote: > https://codereview.appspot.com/314490043/diff/1/lily/one-page-breaking.cc > File lily/one-page-breaking.cc (right): > > https://codereview.appspot.com/314490043/diff/1/lily/one-page-breaking.cc#newcode97 > ...
5 days, 4 hours ago (2017-02-18 20:20:27 UTC) #2
3 days, 22 hours ago (2017-02-20 03:04:55 UTC) #3
On 2017/02/18 20:20:27, dak wrote:
> Actually I didn't.  I just put the finishing touches on a task mainly done by
> Valentin that made it possible to enable a debugging option in Guile, and the
> compiler then dug this one up in consequence as well as a few others.

Ah, cool, nice to have these checks.  I gave it a try with the new change to the
code and it still works as intended.

I think the reason this did not show up as a problem before is that this part of
the code is a defensive measure against a rare edge case where the last musical
system (or top-level-markup) in the data structure does not produce the lowest
visual point of the 'music' on the page (excluding footers, etc.).  

I looked back at the test file I was using locally while working on this, and
found an example that seems to cover this edge case.  However, it appears to
work fine with or without the newest change to the code...  So it's possible
that this code is more defensive than it needs to be, but it's also possible
that removing the defensive measure might lead to bugs in other edge cases, and
it doesn't hurt to have it in there.  So I think it's better to leave it as is
and err on the defensive side.

I can upload a regtest for that edge case in another patch.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted