On 2016/08/12 22:04:52, dak wrote:
I repeat:
Can we get to some version of the code where the code paths supposed
to be
equivalent (is there agreement about that?) actually looks the same?
I think the simplest options here are to (1) make the same changes in both code paths for consistency, or (2) add a comment explaining why "volume" should be initialized to the level of the "mf" dynamic in one of the cases, but not in the other (if there's a specific reason for it).
If so, this would be a good starter for Heikki to eventually propose a
cleanup
that would result in a removal of the dead code or keeping it but
adding a
programming error. Something like that. This sounds like work either party has not bargained on doing.
When this piece of code was introduced in Issue 4048, the code paths did not behave exactly the same way, so I asked about this during the review (https://codereview.appspot.com/302910043/patch/40001/50012), wondering whether the code paths could be combined if the logic is actually supposed to be identical (precisely to keep the code more maintainable in this case by removing any opportunities to forget updating both code paths the same way if changes are needed in one of them), although I didn't propose any concrete rewrite suggestions there. As a result, the logic in both code paths got unified in the final patchset for Issue 4048 (into the state before the current patch), and since the code looked (in my view) to be in a working state afterwards, I did not push any further with the cleanup suggestions since my last comment in that review ended the review discussion. I can certainly try to propose a clean-up patch based on my own understanding about the code, however since the discussion so far seems to suggest that I could simply be missing a trivial critical point about the intended behavior, I'd rather not put effort in preparing a patch that will only end up making things worse. In any case, I think it's best to handle the cleanup as a separate issue so that this one can be closed. https://codereview.appspot.com/308890043/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel