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

Issue 333500043: key-performer.cc: use Pitch::negated for readability

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 2 months ago by dak
Modified:
6 years, 2 months ago
Reviewers:
Dan Eble
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

key-performer.cc: use Pitch::negated for readability This one had been irritating my comprehension, partly because the argument order for pitch_interval in this use case is not readily obvious.

Patch Set 1 #

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

Messages

Total messages: 2
Dan Eble
https://codereview.appspot.com/333500043/diff/1/lily/key-performer.cc File lily/key-performer.cc (right): https://codereview.appspot.com/333500043/diff/1/lily/key-performer.cc#newcode71 lily/key-performer.cc:71: key_do.negated ().smobbed_copy ()); LGTM, but ... I also have ...
6 years, 2 months ago (2018-01-17 00:18:54 UTC) #1
dak
6 years, 2 months ago (2018-01-17 08:41:47 UTC) #2
On 2018/01/17 00:18:54, Dan Eble wrote:
> https://codereview.appspot.com/333500043/diff/1/lily/key-performer.cc
> File lily/key-performer.cc (right):
> 
>
https://codereview.appspot.com/333500043/diff/1/lily/key-performer.cc#newcode71
> lily/key-performer.cc:71: key_do.negated ().smobbed_copy ());
> LGTM, but ...
> 
> I also have a hard time understanding the existing code in this function.
> 
> Isn't it sufficient to check whether the interval between pitch-alist[0] and
> pitch-alist[2] is a minor third, i.e. 3 half-steps?  I don't see why the key
> signature would need to be transposed to C for that.  Maybe there's a
> music-theoretical concern I'm overlooking.

Microtonality?  User-specified signatures/scales?  I am not going to upset stuff
here without good reason.
Sign in to reply to this message.

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