Re: Issue 4433: chord step 14 should be flat by default (issue 253410043 by address@hidden)

[dak]

Reviewers: Dan Eble,


https://codereview.appspot.com/253410043/diff/1/lily/parser.yy
File lily/parser.yy (right):

https://codereview.appspot.com/253410043/diff/1/lily/parser.yy#newcode4061
lily/parser.yy:4061: while (step < 1)
On 2015/07/24 11:56:40, Dan Eble wrote:
> Can't this be done in O(1) with some formula containing a modulo?
(Yes, I see
> it was this way before, but since you're working so close by...)

ANSI C does not specify modulo behavior for negative numbers.  C++ does,
but it's not really "modulo" behavior but "remainder".  As a result, you
need to treat negative numbers specially anyway, and you need to concoct
weird formulas for doing so.  As division is a potentially expensive
operation anyway and input cannot easily be negative either considering
where it is coming from, it seems pointless to add this further
obfuscation for the sake of an efficiency that more likely than not is a
lie.

https://codereview.appspot.com/253410043/diff/1/lily/parser.yy#newcode4064
lily/parser.yy:4064: if (step % 7 == 0)
On 2015/07/24 11:56:40, Dan Eble wrote:
> "I say not unto thee, Until seven times: but, Until seventy times
seven."  :-)

I was considering ((step - 1) % 7 == 6) for clarity but then decided
"what the heck".  Maybe I should really write it that way.  It does seem
more obvious.

Description:
Issue 4433: chord step 14 should be flat by default

Please review this at https://codereview.appspot.com/253410043/

Affected files (+4, -3 lines):
  M lily/parser.yy


Index: lily/parser.yy
diff --git a/lily/parser.yy b/lily/parser.yy
index  
48ada375661dd85f2b44e46e298016c971af5905..23045dcafffa6d2bb26dfbf0356b6b9a29c480aa  
100644
--- a/lily/parser.yy
+++ b/lily/parser.yy
@@ -4058,11 +4058,12 @@ make_chord_step (SCM step_scm, Rational alter)
 {
         int step = scm_to_int (step_scm);

-       if (step == 7)
+       while (step < 1)
+               step += 7;
+
+       if (step % 7 == 0)
                alter += FLAT_ALTERATION;

-       while (step < 0)
-               step += 7;
        Pitch m ((step -1) / 7, (step - 1) % 7, alter);
        return m.smobbed_copy ();
 }