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 _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
