[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (is
From: |
benko . pal |
Subject: |
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068) |
Date: |
Sun, 31 Jul 2011 09:16:02 +0000 |
hi Bertrand,
the patch is correct, AFAICS; see some minor improvements below.
minor concerns (which shouldn't delay acception):
1. about the very existence of usable-duration-logs -
ok, it's generic, but who uses this genericity?
is it not always (0 -1 -2 -3)?
is it not always a range with lower end -3?
is it not always a range?
2. comments, more descriptive names, e.g.
round_up instead of round in calc_measure_duration_log;
measure_count instead of measures (and l),
symbol_count instead of count in church_rest
http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):
http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc#newcode163
lily/multi-measure-rest.cc:163: closest_usable_duration_log =
minimum_usable_duration_log;
I think these two lines can be moved after the loop
http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc#newcode257
lily/multi-measure-rest.cc:257: int mdl = calc_measure_duration_log
(me);
move this before the loop
http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc#newcode268
lily/multi-measure-rest.cc:268: k = mdl + i;
I'd write lines 254-268 like
/*
find the longest usable rest fitting into l:
k identifies a canditate by its duration-log,
length is its duration (in mdl units)
*/
int k = longest_church_rest;
int length = 1 << (mdl - k);
for (; length > l; ++k)
length >>= 1;
l -= length;
http://codereview.appspot.com/4536068/
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), (continued)
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), k-ohara5a5a, 2011/07/27
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), bordage . bertrand, 2011/07/27
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), dak, 2011/07/27
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), mtsolo, 2011/07/27
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), k-ohara5a5a, 2011/07/27
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), bordage . bertrand, 2011/07/28
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), bordage . bertrand, 2011/07/28
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), bordage . bertrand, 2011/07/28
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068),
benko . pal <=
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), bordage . bertrand, 2011/07/31
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), pkx166h, 2011/07/31
- Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068), pkx166h, 2011/07/31