Corrected comments and a function check_meshing_chords divided in two (issue 2310)

Corrected comments and a function check_meshing_chords divided in two.

http://code.google.com/p/lilypond/issues/detail?id=2310

[Keith, 2012-03-31 21:04:35]

LGTM, except that it confuses the two programs we have used recently for
automatic code indentation.

http://codereview.appspot.com/5975054/diff/1/flower/include/direction.hh
File flower/include/direction.hh (right):

http://codereview.appspot.com/5975054/diff/1/flower/include/direction.hh#newc...
flower/include/direction.hh:90: for (Direction d = UP; d != CENTER; flip(&d), d
== UP ? d = CENTER : d)
It is still difficult to understand.  
Consider 
for (Direction d = UP; d != CENTER; d = (UP == d)? DOWN, CENTER)

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode45
lily/note-collision.cc:45: /* Filter out the 'o's in this configuration, since
they're no
> Keith, I'm trying to understand the whole function and 
> divide it into parts.
> Does this comment refer only to the two lines below?

It explains the two assignment lines below.  We use the results of those
assignments, the filtered arrays of note-heads, for the rest of the function.
Therefore you don't need to pass 'ups' and 'dps' as arguments in to this
function.
See http://code.google.com/p/lilypond/issues/detail?id=984

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode404
lily/note-collision.cc:404: for_UP_and_DOWN (d)
The code-indenting program we use
  http://astyle.sourceforge.net/
is not smart enough to determine that this macro starts a control block, so it
indents the code as if for_UP_and_DOWN was a function call or function
definition.
The indenter in emacs does the same.

If you write the 'for' loop directly in the code, then neither humans nor
auto-indent programs need to learn a macro:
for (Direction d = UP; d; d = (UP == d)? DOWN : CENTER)

[Graham Percival, 2012-04-01 05:00:24]

could you split this into 2 (or more) separate patches?

- some of your comments are good, and could have been pushed a month ago.
- the macro changes are highly problematic and probably won't be accepted for at
least a few months.

I never like seeing good changes postponed due to them being squashed together
with questionable ones.

http://codereview.appspot.com/5975054/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/accidental-placement.cc#new...
lily/accidental-placement.cc:116: //TODO: add comment to this struct
this still doesn't add any useful information.  I mean, of course it would be
nice to explain stuff in the code.  But there's no point going around adding
// TODO: comment this
to every single struct/class/method/function.

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode404
lily/note-collision.cc:404: for_UP_and_DOWN (d)
On 2012/03/31 21:04:35, Keith wrote:
> If you write the 'for' loop directly in the code, then neither humans nor
> auto-indent programs need to learn a macro:
> for (Direction d = UP; d; d = (UP == d)? DOWN : CENTER)

Ouch.  I don't find that for loop to be particularly easy to understand; it
would be much nicer if there was a macro for this.

I wonder if we could ask astyle to add a
--custom-loop-commands="for_UP_and_DOWN" option?  it would take a while for that
to reach an official release, but at least there would be some hope of
simplifying this.  I agree that we shouldn't switch to a macro if that ruins the
indentation.

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode577
lily/note-collision.cc:577: for_UP_and_DOWN (d) // please, make a comment to
this loop (better than the above one...)
adding a comment to say "please comment this" does not help

[Keith, 2012-04-01 06:12:26]

On 2012/04/01 05:00:25, Graham Percival wrote:
> Ouch.  I don't find that for loop to be particularly easy to understand; 

No-one has found a clearer way to write this loop in C++.  The best we can do is
choose a consistent idiom that we can learn to recognize.  Any of the suggested
for()-forms hare helpful, so that the so the thing we recognize is at the top of
the loop rather than at the end of a do..while().

> it would be much nicer if there was a macro for this.

It is better to define macros for just the part inside the for(...) .  Then we
can write, and auto-indenters can indent,

for (UP_and_DOWN(d))
  { ... }
for (LEFT_and_RIGHT(d))
  { ... }

However, is a macro nicer ?   
The first time every new contributor sees this loop form, she has to take the
time to understand it.  The macro adds two steps to initial understanding:
realizing that this must be a macro, and then searching for the macro
definition.

[Graham Percival, 2012-04-01 06:26:04]

On Sun, Apr 01, 2012 at 06:12:27AM +0000, k-ohara5a5a@oco.net wrote:
> On 2012/04/01 05:00:25, Graham Percival wrote:
> >it would be much nicer if there was a macro for this.
> 
> It is better to define macros for just the part inside the for(...) .
> Then we can write, and auto-indenters can indent,
> 
> for (UP_and_DOWN(d))
>   { ... }
> for (LEFT_and_RIGHT(d))
>   { ... }

ooh, I like that!  I really, really like it!

> However, is a macro nicer ?

Yes, absolutely.

> The first time every new contributor sees this loop form, she has to
> take the time to understand it.  The macro adds two steps to initial
> understanding: realizing that this must be a macro, and then searching
> for the macro definition.

I disagree.  If there's a macro like that, used everywhere in the
code base, then as a new contributor I'd be happy assuming that it
does what it claims.  If UP_and_DOWN(d) was broken, then surely
there'd be tons of breakage all over the code.  I consider this a
useful abstraction.

Now, there should probably be a paragraph explaining those two
macros in the CG.  But that's a minor detail.

- Graham

[hanwenn, 2012-04-02 01:03:40]

http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh
File lily/include/note-collision.hh (right):

http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh#n...
lily/include/note-collision.hh:56: int down_ball_type;
this doesnt make sense?  The booleans classify the collision type, while these
ints are input data to the problem?  I think it is better to separate input data
and the rest.

http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh#n...
lily/include/note-collision.hh:69: down_ball_type(down_ball_type_) {}
drop this ctor: just have ctor that init everything to default values, and then
use

 ctype.full_collide = true

this is much easier to read, since the order of the 6 args is easy to get wrong.

Is this data type really necessary, btw?

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc
File lily/staff-symbol-referencer.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:126: * Returns vertical position of a symbol
relatively to the staff.
relative

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:137: * The unit is halves of staff space.
was the official coding style for comments changed? If not, can you avoid the
leading *s ?

[Milimetr88, 2012-04-14 13:56:18]

These corrections are included in the Patch Set 2.

http://codereview.appspot.com/5975054/diff/1/flower/include/direction.hh
File flower/include/direction.hh (right):

http://codereview.appspot.com/5975054/diff/1/flower/include/direction.hh#newc...
flower/include/direction.hh:90: for (Direction d = UP; d != CENTER; flip(&d), d
== UP ? d = CENTER : d)
On 2012/03/31 21:04:35, Keith wrote:
> It is still difficult to understand.  
> Consider 
> for (Direction d = UP; d != CENTER; d = (UP == d)? DOWN, CENTER)

Ah, yes, you're right - your suggestion will be more readable. Done.

http://codereview.appspot.com/5975054/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/accidental-placement.cc#new...
lily/accidental-placement.cc:116: //TODO: add comment to this struct
On 2012/04/01 05:00:25, Graham Percival wrote:
> this still doesn't add any useful information.  I mean, of course it would be
> nice to explain stuff in the code.  But there's no point going around adding
> // TODO: comment this
> to every single struct/class/method/function.

Well, ok. I see your point. But how to increase probability that anyone will
comment that struct? Ok, I'll mail the author (Han-Wen) and ask him to comment
this structure, that he created in... 2002.

http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh
File lily/include/note-collision.hh (right):

http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh#n...
lily/include/note-collision.hh:56: int down_ball_type;
On 2012/04/02 01:03:40, hanwenn wrote:
> this doesnt make sense?  The booleans classify the collision type, while these
> ints are input data to the problem?  I think it is better to separate input
data
> and the rest.

Done.

http://codereview.appspot.com/5975054/diff/1/lily/include/note-collision.hh#n...
lily/include/note-collision.hh:69: down_ball_type(down_ball_type_) {}
On 2012/04/02 01:03:40, hanwenn wrote:
> drop this ctor: just have ctor that init everything to default values, and
then
> use
> 
>  ctype.full_collide = true
> 
> this is much easier to read, since the order of the 6 args is easy to get
wrong.
> 
> Is this data type really necessary, btw?

Well, I wanted to split check_meshing_chords. Maybe indeed I should have placed
in the determine_collision_type() only the part after the line
int down_ball_type = Rhythmic_head::duration_log (head_down);

I'll do so.

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode45
lily/note-collision.cc:45: /* Filter out the 'o's in this configuration, since
they're no
On 2012/03/31 21:04:35, Keith wrote:
> > Keith, I'm trying to understand the whole function and 
> > divide it into parts.
> > Does this comment refer only to the two lines below?
> 
> It explains the two assignment lines below.  We use the results of those
> assignments, the filtered arrays of note-heads, for the rest of the function.
> Therefore you don't need to pass 'ups' and 'dps' as arguments in to this
> function.
> See http://code.google.com/p/lilypond/issues/detail?id=984

Oh, well, it's my mistake - indeed dps is not needed, *BUT* ups is -
unfortunately there is one place where it is used after being filtered (line
230), so ups should have been passed by reference not by value, right? 
I find it ugly to pass something by reference in such a function, but don't see
a better solution.

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode238
lily/note-collision.cc:238: /* The solfa is a triangle, which is inverted
depending on stem
What does solfa mean? I couldn't find it on Google...

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode404
lily/note-collision.cc:404: for_UP_and_DOWN (d)
On 2012/04/01 05:00:25, Graham Percival wrote:
> On 2012/03/31 21:04:35, Keith wrote:
> > If you write the 'for' loop directly in the code, then neither humans nor
> > auto-indent programs need to learn a macro:
> > for (Direction d = UP; d; d = (UP == d)? DOWN : CENTER)
> 
> Ouch.  I don't find that for loop to be particularly easy to understand; it
> would be much nicer if there was a macro for this.
> 
> I wonder if we could ask astyle to add a
> --custom-loop-commands="for_UP_and_DOWN" option?  it would take a while for
that
> to reach an official release, but at least there would be some hope of
> simplifying this.  I agree that we shouldn't switch to a macro if that ruins
the
> indentation.

Ok, could I assume that you will take care of modyfing astyle? I have no
experience with it and instead of learning it now, I would like to answer all
comments and correct all glitches, and finally close this patch.

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode577
lily/note-collision.cc:577: for_UP_and_DOWN (d) // please, make a comment to
this loop (better than the above one...)
On 2012/04/01 05:00:25, Graham Percival wrote:
> adding a comment to say "please comment this" does not help

Once again, what could be done to get a comment to that loop?

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc
File lily/staff-symbol-referencer.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:126: * Returns vertical position of a symbol
relatively to the staff.
On 2012/04/02 01:03:40, hanwenn wrote:
> relative
Relative to which element?

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:137: * The unit is halves of staff space.
On 2012/04/02 01:03:40, hanwenn wrote:
> was the official coding style for comments changed? If not, can you avoid the
> leading *s ?

Is there any official coding style for comments? I couldn't find any in CG.

And as it was stated here:
http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod...
leading *'s prevent comments misalignment.

[Graham Percival, 2012-04-14 14:18:58]

On 2012/04/14 13:56:18, Milimetr88 wrote:
> On 2012/03/31 21:04:35, Keith wrote:
> > It is still difficult to understand.  
> > Consider 
> > for (Direction d = UP; d != CENTER; d = (UP == d)? DOWN, CENTER)
> 
> Ah, yes, you're right - your suggestion will be more readable. Done.

You don't like the
for ( UP_and_DOWN(d) )
macro?  I think that's *far* more readable than Keith's version.

> > // TODO: comment this
> > to every single struct/class/method/function.
> 
> Well, ok. I see your point. But how to increase probability that anyone will
> comment that struct?

Nobody's going to comment that struct, whether or not you clutter up the source
code with a //TODO comment.

I mean, look at this:
gperciva@gperciva-desktop:~/src/lilypond (master)$ git grep FIXME | wc -l
286
gperciva@gperciva-desktop:~/src/lilypond (master)$ git grep TODO | wc -l
979

Making that last number into 980 is not going to help.

> > I wonder if we could ask astyle to add a
> > --custom-loop-commands="for_UP_and_DOWN" option?  it would take a while for
> 
> Ok, could I assume that you will take care of modyfing astyle?

No, but putting the macro inside the for() loop means that we don't need to.

> I have no
> experience with it and instead of learning it now, I would like to answer all
> comments and correct all glitches, and finally close this patch.

I _really_ suggest that you split the patch into separate patches.  Some of
these changes are good and could have been pushed weeks ago; others will still
provoke arguments for the next few weeks.

> http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode577
> lily/note-collision.cc:577: for_UP_and_DOWN (d) // please, make a comment to
> this loop (better than the above one...)
> On 2012/04/01 05:00:25, Graham Percival wrote:
> > adding a comment to say "please comment this" does not help
> 
> Once again, what could be done to get a comment to that loop?
 
If you write up what you know about that particular situation and send it to
-devel for comments or questions, that may provide enough incentive for somebody
to explain it to you, and then you could add a comment there.

 
>
http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
> lily/staff-symbol-referencer.cc:137: * The unit is halves of staff space.
> On 2012/04/02 01:03:40, hanwenn wrote:
> > was the official coding style for comments changed? If not, can you avoid
the
> > leading *s ?
> 
> Is there any official coding style for comments? I couldn't find any in CG.

Hmm, I'm not certain if we have one or not.
 
> And as it was stated here:
>
http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod...
> leading *'s prevent comments misalignment.

That sounds like a very good idea!

- Graham

[Milimetr88, 2012-04-14 14:31:24]

http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc#...
lily/accidental-placement.cc:116: //TODO: add comment to this struct
I'll delete that.

[Milimetr88, 2012-04-14 14:36:23]

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode584
lily/note-collision.cc:584: || (extents[-d].size () && d * (extents[d][i][-d] -
extents[-d][0][d]) < 0))   // please, comment this condition
I'll delete that one day ;)

[Milimetr88, 2012-04-14 14:39:36]

On 14 April 2012 16:18, <graham@percival-music.ca> wrote:

>
>
> I have no
>> experience with it and instead of learning it now, I would like to
>>
> answer all
>
>> comments and correct all glitches, and finally close this patch.
>>
>
> I _really_ suggest that you split the patch into separate patches.  Some
> of these changes are good and could have been pushed weeks ago; others
> will still provoke arguments for the next few weeks.


To be clear: I believe that I have just stripped all for_UP_and_DOWN from
that patch. I'm planning to add it as a separate issue @Google and upload a
separate patch @Rietveld and that's what I will discuss in a separate mail.

> // TODO: comment this
>> > to every single struct/class/method/function.
>>
>
>  Well, ok. I see your point. But how to increase probability that
>>
> anyone will
>
>> comment that struct?
>>
>
> Nobody's going to comment that struct, whether or not you clutter up the
> source code with a //TODO comment.
>

Ok, I'll delete that.


> I mean, look at this:
> gperciva@gperciva-desktop:~/**src/lilypond (master)$ git grep FIXME | wc
> -l
> 286
> gperciva@gperciva-desktop:~/**src/lilypond (master)$ git grep TODO | wc -l
> 979
>
> Making that last number into 980 is not going to help.


Well, partially you're right. But I think that it's not the problem that
someone *adds* a new TODO. The problem is that *noone will handle them*!
Maybe somebody should? I can take a look on those TODOs and FIXMEs, but
don't promise that I will be able to correct them - many of them probably
need pretty much knowledge of Lilypond internals, which I don't have. Yet ;)


> No, but putting the macro inside the for() loop means that we don't need
> to.

Ok, I'll do a macro UP_and_DOWN.

http://codereview.appspot.com/**5975054/diff/1/lily/note-**
>
collision.cc#newcode577<http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode577>
>
>> lily/note-collision.cc:577: for_UP_and_DOWN (d) // please, make a
>>
> comment to
>
>> this loop (better than the above one...)
>> On 2012/04/01 05:00:25, Graham Percival wrote:
>> > adding a comment to say "please comment this" does not help
>>
>
>  Once again, what could be done to get a comment to that loop?
>>
>
> If you write up what you know about that particular situation and send
> it to -devel for comments or questions, that may provide enough
> incentive for somebody to explain it to you, and then you could add a
> comment there.


Ok, I'll do so. I'll mark it in my own code as TODO.


>  http://codereview.appspot.com/**5975054/diff/1/lily/staff-**
>
symbol-referencer.cc#**newcode137<http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#newcode137>
>
>> lily/staff-symbol-referencer.**cc:137: * The unit is halves of staff
>>
> space.
>
>> On 2012/04/02 01:03:40, hanwenn wrote:
>> > was the official coding style for comments changed? If not, can you
>>
> avoid the
>
>> > leading *s ?
>>
>
>  Is there any official coding style for comments? I couldn't find any
>>
> in CG.
>
> Hmm, I'm not certain if we have one or not.


Who knows if not you? :)


> And as it was stated here:
>>
>
> http://codereview.appspot.com/**5651069/diff/5002/lily/note-**
>
collision.cc#newcode191<http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcode191>
>
>> leading *'s prevent comments misalignment.
>>
>
> That sounds like a very good idea!
>

Great! It seems like a good thing to be mentioned in Documentation, don't
you think?

Łukasz

[dak, 2012-04-14 14:52:33]

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?

[Milimetr88, 2012-04-14 15:21:26]

On 14 April 2012 16:52, <dak@gnu.org> 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

[dak, 2012-04-14 17:01:36]

On 2012/04/14 15:21:26, Milimetr88 wrote:
> > 
> I didn't expected so long reply. The whole patch is about adding some
> comments to the code, splitting *one* function in two

But you split the function in two at an arbitrary point in the middle, package
all of its local variables from the first half into
a class structure, and then pass this class structure into the second half of
the function.

Where is the point?  Is either half of the function used for some other purpose
as well?  _Could_ it even be used for some other purpose?  What is the purpose
of creating this function border and parameter-passing API?

[Keith, 2012-04-14 19:12:01]

Splitting the function in two doesn't make it any easier for me to understand,
but I had figured it out before.

On 2012/03/21 18:56:08, Milimetr88 wrote:
> What I was taught at the university is to write short 
> and simple functions that do only one thing.

Maybe this was intended as advice for when you initially write code; it would
encourage the writer to find the smallest independent tasks and cleanest
interfaces between those tasks.  Splitting up an existing function, that has
grown into its assigned task and assigned interface, is different.

I can find no better place to divide the function than where you have: one
function to characterize the meshing of note-heads and the rest to determine how
to shift the chords.

The interface is complicated because pass several properties of the two chords. 
Maybe in determine_collision_type() you could just look them up again through
the two Stems?

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode45
lily/note-collision.cc:45: /* Filter out the 'o's in this configuration, since
they're no
On 2012/04/14 13:56:18, Milimetr88 wrote:
> indeed dps is not needed, *BUT* ups is - unfortunately 

The use of ups[0] on line 230 is correct(*) whether or not suspended notes are
included.  Passing the variable between functions is not required.
*-correct for all the cases LilyPond handles to date; David will generalizing
this code shortly.

http://codereview.appspot.com/5975054/diff/1/lily/note-collision.cc#newcode238
lily/note-collision.cc:238: /* The solfa is a triangle, which is inverted
depending on stem
On 2012/04/14 13:56:18, Milimetr88 wrote:
> What does solfa mean? 
Solfège.  Here it was probably a typo for "The fa is a triange"

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc
File lily/staff-symbol-referencer.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:126: * Returns vertical position of a symbol
relatively to the staff.
On 2012/04/14 13:56:18, Milimetr88 wrote:
> Relative to which element?
"relative to the staff"
Most English speakers think of this construction as an adjective, describing
'position', so we use the adjective form 'relative'.
We see 'relatively' a lot from people who think in other languages, but that
makes English speakers search for the adjective being modified by 'relatively'.

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:137: * The unit is halves of staff space.
On 2012/04/14 13:56:18, Milimetr88 wrote:
> 
> Is there any official coding style for comments? I couldn't find any in CG.

With very few exceptions, block comments in LilyPond C code use no *.  That is
enough to recognize the convention.  You should take out the *s here.

> And as it was stated here:
>
http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod...
> leading *'s prevent comments misalignment.

Those are the exceptions.  Emacs' indenter will left-align block comments. Years
ago someone auto-indented the code with emacs and destroyed all the ASCII-art
comments that draw note-configurations.

If we write an explicit rule, then it is harder to adapt the next time we meet
an exceptional case.

[dak, 2012-04-14 20:37:06]

On 2012/04/14 19:12:01, Keith wrote:
> Splitting the function in two doesn't make it any easier for me to understand,
> but I had figured it out before.
> 
> On 2012/03/21 18:56:08, Milimetr88 wrote:
> > What I was taught at the university is to write short 
> > and simple functions that do only one thing.

We don't have a short and simple task.  A function defines boundaries.  It has
well-defined input, processes the input using local algorithms and temporary
data, and generates well-defined output.

Splitting the function into two parts does not make sense since the first part
has no well-defined output that can be considered reasonably independent from
the requirements and workings of the algorithms in the second part.  When you
are redesigning the second part, you'll need to redefine the "interface" between
the two parts and the first part as well.  Whether or not you put an artificial
function call boundary in the middle of the function, it is not composed of
modular parts that could be reused in different contexts.

Modularity is not a self-serving goal.  In university, you will often train
using toy problems in order to get anything finished at all.  So one tends to
overmodularize, independent of any actual reuse of functions (the toy problems
usually are small enough that reuse is not an actual issue even where a subtask
_could_ be sensibly modularized).

[Milimetr88, 2012-04-15 13:52:36]

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc
File lily/staff-symbol-referencer.cc (right):

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:126: * Returns vertical position of a symbol
relatively to the staff.
On 2012/04/14 19:12:01, Keith wrote:
> On 2012/04/14 13:56:18, Milimetr88 wrote:
> > Relative to which element?
> "relative to the staff"
> Most English speakers think of this construction as an adjective, describing
> 'position', so we use the adjective form 'relative'.
> We see 'relatively' a lot from people who think in other languages, but that
> makes English speakers search for the adjective being modified by
'relatively'.

Ah, ok. I thought that Han-Wen wants me to replace THE WHOLE comment with a one
word: "relative" :D Now it's clear for me - I'll change that one word.

http://codereview.appspot.com/5975054/diff/1/lily/staff-symbol-referencer.cc#...
lily/staff-symbol-referencer.cc:137: * The unit is halves of staff space.
On 2012/04/14 19:12:01, Keith wrote:
> On 2012/04/14 13:56:18, Milimetr88 wrote:
> > 
> > Is there any official coding style for comments? I couldn't find any in CG.
> 
> With very few exceptions, block comments in LilyPond C code use no *.  That is
> enough to recognize the convention.  You should take out the *s here.
> 
> > And as it was stated here:
> >
>
http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcod...
> > leading *'s prevent comments misalignment.
> 
> Those are the exceptions.  Emacs' indenter will left-align block comments.
Years
> ago someone auto-indented the code with emacs and destroyed all the ASCII-art
> comments that draw note-configurations.
> 
> If we write an explicit rule, then it is harder to adapt the next time we meet
> an exceptional case.

Ok, so there should be leading *, if and only if there is an ASCII-art in the
comment, right?

[Milimetr88, 2012-04-15 13:58:16]

On 14 April 2012 21:12, <k-ohara5a5a@oco.net> wrote:

> Splitting the function in two doesn't make it any easier for me to
> understand, but I had figured it out before.
>
> On 2012/03/21 18:56:08, Milimetr88 wrote:
>
>> What I was taught at the university is to write short
>> and simple functions that do only one thing.
>>
>
> Maybe this was intended as advice for when you initially write code; it
> would encourage the writer to find the smallest independent tasks and
> cleanest interfaces between those tasks.  Splitting up an existing
> function, that has grown into its assigned task and assigned interface,
> is different.




On 14 April 2012 22:37, <dak@gnu.org> wrote:

>
> Splitting the function into two parts does not make sense since the
> first part has no well-defined output that can be considered reasonably
> independent from the requirements and workings of the algorithms in the
> second part.  When you are redesigning the second part, you'll need to
> redefine the "interface" between the two parts and the first part as
> well.  Whether or not you put an artificial function call boundary in
> the middle of the function, it is not composed of modular parts that
> could be reused in different contexts.
>
> Modularity is not a self-serving goal.
>
http://codereview.appspot.com/**5975054/<http://codereview.appspot.com/5975054/>
>

Ok, I'll merge it back.

Łukasz

[Milimetr88, 2012-04-24 17:36:27]

http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/5975054/diff/8001/lily/accidental-placement.cc#...
lily/accidental-placement.cc:116: //TODO: add comment to this struct
On 2012/04/14 14:31:24, Milimetr88 wrote:
> I'll delete that.

Done.