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

[address@hidden]

On 11 nov. 2012, at 14:50, address@hidden 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