Re: Corrected comments and a function check_meshing_chords divided in two. (issue 5975054)

[milimetr88]

Reviewers: Keith, Graham Percival, hanwenn,

Message:
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#newcode90
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#newcode116
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#newcode56
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#newcode69
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#newcode126
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#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.

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

Description:
Corrected comments and a function check_meshing_chords divided in two.

Please review this at http://codereview.appspot.com/5975054/

Affected files:
  M flower/include/direction.hh
  M lily/accidental-placement.cc
  M lily/grob.cc
  M lily/include/grob-interface.hh
  M lily/include/note-collision.hh
  M lily/note-collision.cc
  M lily/note-column.cc
  M lily/staff-symbol-referencer.cc