On 11 nov. 2012, at 14:50, d...@gnu.org wrote: > > 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?
It is a verb. "own dimensions" means that the dimensions of the object itself are taken into account (as opposed to just the offset). > > 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? I'm getting the sense that what you want to do is set up a commenting style for the entire code base. I'd recommend starting a discussion about this. We could establish a rule that there must be a comment for every argument going to every function (this is perhaps not a bad idea) but the code is full of undocumented functions and arguments. So it seems like an issue to tackle apart. > > 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. Do a git grep for MAKE_SCHEME_CALLBACK_WITH_OPTARGS and you'll see that the majority are un-doc-stringed. Again, it sounds like you want to put in place an overarching system for comments and documentation. This is not a bad idea, but I think it needs to be addressed as a separate issue. I don't mind at all your using this patch as a test-case, but I'd recommend first establishing a policy and then going through the code base and applying it everywhere. > > 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. 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. 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 See above. > b) no documentation what each step does. > Every time there is an important new thing happening there is a comment. > 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? Sorry, the problem is the word "apparent". There is actually no logic to it, period. It is an arbitrary decision that allowed InstrumentName to work. I will reword it to make it clear that the problem is that 0 is arbitrary. > > 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? I'm not sure who originally wrote this comment - I understand it to mean that the number 1000 is a magic number that should change with the size of the paper being used. To be clear, I have no problem implementing a LilyPond documentation and commenting policy to move towards the establishment of an API. This sounds like a worthwhile yet difficult task. I would start talking about this separately from any one patch. Of course, this does not exempt patches from using comments. I feel that my comments in this patch are clear for someone who reads the code with them. What you're talking about is something more systematic, which is a great idea and worth opening a tracker issue about. Cheers, MS _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel