[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: milimetr88
Subject: Re: Corrected comments and a function check_meshing_chords divided in two. (issue 5975054)
Date: Sat, 14 Apr 2012 13:56:18 +0000

Reviewers: Keith, Graham Percival, hanwenn,

These corrections are included in the Patch Set 2.
File flower/include/direction.hh (right):
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.
for (Direction d = UP; d != CENTER; d = (UP == d)? DOWN, CENTER)

Ah, yes, you're right - your suggestion will be more readable. Done.
File lily/ (right):
lily/ //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
// 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.
File lily/include/note-collision.hh (right):
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.

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

  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.
File lily/ (right):
lily/ /* 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
assignments, the filtered arrays of note-heads, for the rest of the
Therefore you don't need to pass 'ups' and 'dps' as arguments in to

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,
I find it ugly to pass something by reference in such a function, but
don't see a better solution.
lily/ /* The solfa is a triangle, which is
inverted depending on stem
What does solfa mean? I couldn't find it on Google...
lily/ 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

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
lily/ 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?
File lily/ (right):
lily/ * Returns vertical position of a
symbol relatively to the staff.
On 2012/04/02 01:03:40, hanwenn wrote:
Relative to which element?
lily/ * The unit is halves of staff
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

And as it was stated here:
leading *'s prevent comments misalignment.

Corrected comments and a function check_meshing_chords divided in two.

Please review this at

Affected files:
  M flower/include/direction.hh
  M lily/
  M lily/
  M lily/include/grob-interface.hh
  M lily/include/note-collision.hh
  M lily/
  M lily/
  M lily/

reply via email to

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