I would suggest reverting this patch as "needs work" for now.
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode125 lily/multi-measure-rest.cc:125: SCM sml = dynamic_cast<Spanner *> (me)->get_bound (LEFT) There is no good way to get around the cast? http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode130 lily/multi-measure-rest.cc:130: double duration_log = -log2 (ml.Rational::to_double ()); log2 is a floating point operation not guaranteed to be exact. One usually uses a loop for figuring out a proper integer value. It is also not clear to me that this will pick out the right kind of rest always. For example, what to do about 3/2? We either get 2/1 rests, or 1/1 rests, and it is not clear to me that this rounding choice is necessarily the same as that for 3/4. I think it might be saner to just have an overrideable lookup list for exact meters (and 4/4 is not necessarily the same as 2/2 here), and revert to a separately configurable default otherwise, likely a whole rest. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode140 lily/multi-measure-rest.cc:140: int list_elt = scm_to_int (scm_list_ref (duration_logs_list, scm_from_int (i))); Iterating through a list as if it were an array is a no-no. This makes the operation O(n^2) instead of O(n) and obfuscates what happens. Check other code examples for how to iterate through a list. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode229 lily/multi-measure-rest.cc:229: for (int i = 0; i < scm_to_int (scm_length (duration_logs_list)); i++) Again: don't loop through a list by indexing it like an array. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode246 lily/multi-measure-rest.cc:246: length = int (pow (2, -i)); Don't use floating point arithmetic. This is just 2<<-i unless i is positive. One should exit the loop _before_ that happens, otherwise length will be undefined with the integer variant. With the floating point variant, length becomes 0 before exiting the loop with i==1. I doubt that is intentional. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode254 lily/multi-measure-rest.cc:254: Stencil r (musfont->find_by_name ("rests." + to_string (k))); This may calculate a name "rests.-1" instead of the valid "rests.M1". Use Rest::glyph_name instead, though it may also need fixing in that regard. http://codereview.appspot.com/4536068/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
