Re: Issue 4936: look up "mf" for default initial volume (issue 308890043 by address@hidden)

[ht . lilypond . development]

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/