lilypond-devel
[Top][All Lists]
Advanced

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

Uses single algorithm for side-position spacing. (issue 6827072)


From: dak
Subject: Uses single algorithm for side-position spacing. (issue 6827072)
Date: Sun, 11 Nov 2012 13:50:14 +0000


http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc
File lily/side-position-interface.cc (right):

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode85
lily/side-position-interface.cc:85: Position next to support, taking
into account my own dimensions and padding.
Incomprehensible comment.  Is position verb or noun?  What do your own
dimensions have to do with it?

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode88
lily/side-position-interface.cc:88: axis_aligned_side_helper (SCM smob,
Axis a, bool pure, int start, int end, SCM current_off_scm)
Why is the meaning of the arguments undocumented?

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode101
lily/side-position-interface.cc:101: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, x_aligned_side, 2, 1, "");
It is bad enough if you omit all comments from the code, but if the
declaration of a function _explicitly_ includes a DOC string, specifying
this as "" for a function with a vague name is not really helpful.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode108
lily/side-position-interface.cc:108: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, y_aligned_side, 2, 1, "");
See above.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode115
lily/side-position-interface.cc:115: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, pure_y_aligned_side, 4, 1, "");
See above.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode144
lily/side-position-interface.cc:144: // long function - each stage is
clearly marked
Too bad that there is
a) no documentation what the long function does
b) no documentation what each step does.

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode275
lily/side-position-interface.cc:275: // necessary for the InstrumentName
grob
Well, if there is no _apparent_ logic to it, that would seem to warrant
explaining the logic, wouldn't it, instead of proudly announcing another
puzzle to the reader?

http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode309
lily/side-position-interface.cc:309: /* FIXME: 1000 should relate to
paper size.  */
FIXME: the meaning of no variable at all is documented, so how should
the reader even know this is "paper size" related?

http://codereview.appspot.com/6827072/



reply via email to

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