[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Corrected comments and a function check_meshing_chords divided in tw

From: Łukasz Czerwiński
Subject: Re: Corrected comments and a function check_meshing_chords divided in two. (issue 5975054)
Date: Sat, 14 Apr 2012 17:21:05 +0200

On 14 April 2012 16:52, <address@hidden> 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?

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]