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

[janek . lilypond]

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/