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

Issue 11052: Fix key signatures with accidentals in specific octave. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 10 months ago by Neil Puttock
Modified:
10 years, 2 months ago
Reviewers:
joeneeman
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fix key signatures with accidentals in specific octave. - when testing whether a pitch matches the key signature, try using keySignature if no match is found in localKeySignature. - move check_pitch_against_signature () to SCM, passing context instead of localKeySignature - remove ly:find-accidentals-simple

Patch Set 1 #

Patch Set 2 : Move check-pitch-against-signature to SCM #

Patch Set 3 : Remove unused function recent_enough (). #

Total comments: 4

Patch Set 4 : Improvements based on Joe's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+134 lines, -148 lines) Patch
A input/regression/key-signature-scordatura-persist.ly View 1 1 chunk +19 lines, -0 lines 0 comments Download
M lily/accidental-engraver.cc View 1 2 5 chunks +16 lines, -144 lines 0 comments Download
M scm/music-functions.scm View 1 2 3 2 chunks +99 lines, -4 lines 0 comments Download

Messages

Total messages: 5
joeneeman
lgtm, except for the description "move check_pitch_against_signature () to SCM," since ly:check-pitch-against-signature is still implemented ...
10 years, 7 months ago (2009-03-24 23:06:42 UTC) #1
Neil Puttock
On 2009/03/24 23:06:42, joeneeman wrote: > lgtm, except for the description "move check_pitch_against_signature () to ...
10 years, 7 months ago (2009-03-24 23:24:30 UTC) #2
Neil Puttock
On 2009/03/24 23:06:42, joeneeman wrote: > lgtm, except for the description "move check_pitch_against_signature () to ...
10 years, 6 months ago (2009-04-16 19:50:57 UTC) #3
joeneeman
http://codereview.appspot.com/11052/diff/3409/2410 File scm/music-functions.scm (right): http://codereview.appspot.com/11052/diff/3409/2410#newcode1047 Line 1047: ((and (equal? ignore-octave #f) I think eq? is ...
10 years, 6 months ago (2009-04-17 19:25:31 UTC) #4
Neil Puttock
10 years, 6 months ago (2009-04-17 20:59:07 UTC) #5
On 2009/04/17 19:25:31, joeneeman wrote:
> http://codereview.appspot.com/11052/diff/3409/2410
> File scm/music-functions.scm (right):
> 
> http://codereview.appspot.com/11052/diff/3409/2410#newcode1047
> Line 1047: ((and (equal? ignore-octave #f)
> I think eq? is more appropriate here

Done.

> http://codereview.appspot.com/11052/diff/3409/2410#newcode1048
> Line 1048: (and
> use (and a b c) instead of (and a (and b c))

Done.

> http://codereview.appspot.com/11052/diff/3409/2410#newcode1073
> Line 1073: (if (and (not (= (sign this-alt) 0))
> Surely (= (sign x) 0) is the same as (= x 0)

Of course!

I should have thought a bit more about it rather than trying to duplicate the
C++ code exactly.

> http://codereview.appspot.com/11052/diff/3409/2410#newcode1075
> Line 1075: (< (sign (* prev-alt this-alt)) 0)))
> Surely (< (sign x) 0) is the same as (< x 0)

Yes. :)
Sign in to reply to this message.

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