[Top][All Lists]

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

Re: some comments and complaints on the code (issue 5651069)

From: Carl . D . Sorensen
Subject: Re: some comments and complaints on the code (issue 5651069)
Date: Fri, 10 Feb 2012 23:49:24 +0000

Thanks for taking this on, Janek.

I don't know what the response will be to for_UP_and_DOWN(d).  The last
time somebody proposed a change, it was resisted because the do{}
flip(d)!=UP idiom seemed simple enough to be acceptable.

But I think the new idiom is more readable, and if there are no
performance issues with it, I'd be in favor of it.

I have some specific comments below about spacing, comment format, and
comment placement.  In general, I think it's a good idea for you to add
comments.  I'm not sure it's a good idea to add TODO about missing
comments (but it might be).  I'm really hesitant to have you add
comments that say "please add a comment", since it's not clear to whom
the please is directed.


File lily/include/note-column.hh (right):
lily/include/note-column.hh:39: /**
IMO, this comment belongs in the definition, not the declaration
File lily/include/staff-symbol-referencer.hh (right):
lily/include/staff-symbol-referencer.hh:53: /**
Again, comment in the definition, not the declaration
File lily/ (right):
lily/ ->set_property ("direction", scm_from_int
I think the indentation in the old code is correct.  The -> should be
indented, since it's not the start of a statement.

But if this spacing is what's produced by, then we should keep
lily/ /**
I don't like the double * following the /.  Is this a standard anywhere
else in lilypond?
lily/ for_UP_and_DOWN (d) // please, make a
comment to this loop (better than the above one...)
To whom is the "please" directed?  Is there anybody who is better than
you right now to comment the loop?
lily/ {
I don't know that we have a specification for this, or if it can be
handled by, but for consistency with the way we indent braces
with loops and if, the braces should be indented two spaces, IMO.

reply via email to

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