[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Uses single algorithm for side-position spacing. (issue 6827072)
From: |
address@hidden |
Subject: |
Re: Uses single algorithm for side-position spacing. (issue 6827072) |
Date: |
Sun, 11 Nov 2012 16:26:43 +0100 |
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
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/12
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/17