On 2012/02/10 23:49:24, Carl wrote:
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.
It took us a while to figure out what's going on with the do{}
flip(d)!=UP thing.
If it was up to me, i'd just write everything twice, following the rule
"1, 2, many" :)
Some of your comments will be better addressed by Luke - i leave them to
him.
Thanks for review!
Janek
http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):
http://codereview.appspot.com/5651069/diff/1/lily/accidental-placement.cc#newcode211
lily/accidental-placement.cc:211: * @return A vector of
Accidental_placement_entrys
On 2012/02/11 00:09:00, joeneeman wrote:
Please don't just comment on the type (unless it's SCM in which case
you can say
which scheme type it is).
Well, the main point of the comment is not describing parameters, but
the function itself. Believe me or not, we spent 10 minutes figuring
out _what the hell_ are apes doing here and whether there are any
bananas involved.
It's stupid, i know. But there must exist a way of writing code that is
understandable for the newcomers after second reading, not after 10
minutes.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc
File lily/note-collision.cc (right):
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode316
lily/note-collision.cc:316: shift_amount *= 1.25;
this must be a mistake actually. We didn't intend to do anything with
the working code yet.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode369
lily/note-collision.cc:369: ->set_property ("direction", scm_from_int
(dir));
On 2012/02/10 23:49:24, Carl wrote:
I think the indentation in the old code is correct. The -> should be
indented,
since it's not the start of a statement.
+1
But if this spacing is what's produced by fixcc.py, then we should
keep it.
It was done by fixcc indeed.
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode552
lily/note-collision.cc:552: for_UP_and_DOWN (d) // please, make a
comment to this loop (better than the above one...)
On 2012/02/10 23:49:24, Carl wrote:
To whom is the "please" directed? Is there anybody who is better than
you right
now to comment the loop?
Yes: the author of the code. There are also other people in our team
who will understand this at second reading; i still don't understand how
exactly this works.
(of course i don't demand that anyone does anything about it)
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode588
lily/note-collision.cc:588: {
On 2012/02/10 23:49:24, Carl wrote:
I don't know that we have a specification for this, or if it can be
handled by
fixcc.py, but for consistency with the way we indent braces with loops
and if,
the braces should be indented two spaces, IMO.
+10
However, the result was produced with fixcc. At the moment i don't know
how to fix fixcc; i've looked at its code and it contains a lot of
('([\w\)\]]) *(--|\+\+)', '\\1\\2')
which look nice but i have absolutely no idea how they work, and no time
to learn at the moment :(
http://codereview.appspot.com/5651069/
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
