On 14 April 2012 16:52, <[email protected]> wrote:
> Without wanting to rain on your parade: what is all this about? I
> actually had been planning to fix this collision stuff: there are
> several fundamental shortcomings. The code can't deal sensibly with
> different noteheads (harmonics, mixed duration chords which are required
> for some string music) as it picks some information just off the first
> notehead. It does not cleanly distinguish stem duration and notehead
> duration. It uses rounded notehead positions for its decisions instead
> of the actual notehead positions (possibly shifted, partly as a
> consequence of optical corrections). It does not compare all the
> relevant data of noteheads to be merged, nor does it take into account
> different merging possibilities.
>
> All of this has been implemented with ad-hoc variables and ad-hoc code.
>
> It does appear that you now "C++ize" both variables and code, creating
> artifical APIs for something that is, at its heart, just ad-hoc.
>
> This makes it harder to actually fix the deficiencies of the code. I
> don't see that you address the deficiencies: instead you change the
> style to a style that is harder to maintain and understand because now,
> to change its works, you need to look at the newly introduced APIs and
> change those as well as its uses, for code and data that is actually
> quite localized.
>
> What is the gain from this additional layering? The way it looks to me,
> this will make the code harder to maintain (and fix) than it was
> previously. Can you describe the motivation behind your work?
>
> http://codereview.appspot.com/**5975054/<http://codereview.appspot.com/5975054/>
>
I didn't expected so long reply. The whole patch is about adding some
comments to the code, splitting *one* function in two and adding *one* macro
(for_UP_and_DOWN or for(UP_and_DOWN) ) to be used instead of:
Direction d = UP;
do {
...
...
} while(flip(&d) != UP);
because I find the do-while loop with a strange call to flip() counter
intuitive.
Summing up all the above, initially the main change of the patch was
changing a do-while loop with flip() to a macro (which will be included in
a different patch and talked about in a different mail - with topic
"for_UP_and_DOWN"). The do-while-flip construction is used in 150 places,
so it's not a local change. But in the course of making changes, the patch
ended up with changing only a few comments and splitting a function.
Łukasz
_______________________________________________
lilypond-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
