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