Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)

[dak]

On 2013/02/03 08:45:13, mike7 wrote:
> On 1 févr. 2013, at 16:04, mailto:address@hidden wrote:

> > As previously, there are no comments whatsoever putting the code
into
> > perspective.  This is an amorphous heap of code without an attempt
of
> > explaining its design or inner logic.  There is a single function
> > comment giving a glimpse of what that function is supposed to do.
> > However, there is no explanation of the context this function is
> > supposed to be used in or for, the function naming bears no relation
to
> > its return value, the comment is unclear, half of the named entities
> > don't exist in the function interface, and half of the existing
entities
> > are not mentioned.

> Thanks for the review - comments are addressed below.
> The code exists in order to use Y-offsets of grobs, via the side
position
> interface, to do outside staff placement instead of translate axis.

What does "instead of translate axis" mean?

> It seems to me that a reasonable default is "if no direction property
is set,
> assume that the grob has the same direction as its Y-parent."

That assumes that the direction is always specified relative to the
same entity, and that a child is always placed further from the axis
than its parent.  I am not convinced of that.


https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode461
> > lily/side-position-interface.cc:461: MAKE_SCHEME_CALLBACK
> > (Side_position_interface, outside_staff_aligned_side, 1);
> > Sure, something called "outside_staff_aligned_side" is so obvious in
> > meaning that we don't need to explain what arguments it gets and
what
> > the meaning of the returned value would be.  Just for the record: I
am
> > being sarcastical here.
> >
> > Mike, how is _anybody_ going to be able to understand _anything_
using
> > an undocumented callback with that name?

> Comment added.

That's good to have, but no substitute for a sensible name.  If you
return a shift or offset, the function needs to be named accordingly.
"outside_staff_aligned_side" does not jibe with something returning a
Real value.


https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode481
> > lily/side-position-interface.cc:481:
> > Side_position_interface::calc_outside_staff_offset (Grob *me, Axis
a,
> > bool pure, int start, int end, Real current_offset)
> > Now here is a function name that gives an _inkling_ of a clue what
the
> > returned value is: an offset.  It still is totally undocumented with
> > regard to the meaning of its arguments.

> Comment added.  bool pure, int start, int end are almost never
documented
> anywhere.

It's perfectly fine to just state "standard arguments for grob
callbacks, see xxx.xxx for explanation".  However, I think that "bool
pure" is an invention of your own, and that makes it likely that there
is no explanation anywhere to be found.  I'd definitely prefer one in
a central place, and just a crossreference here.

That does not mean that I prefer a patch that leaves out all
information on the assumption that somebody, sometime, somewhere will
provide the missing parts.

> Ditto for Grob *me.  I will do my best to get into the practice of
> holding myself to the standard of documenting arguments and return
> values for every new function I add.  But I think it is overkill.

It is overkill _if_ there is a central point where they are documented
_and_ you refer there.  It is overkill _if_ the function is named in a
manner that the meaning is obvious _and_ matching a family of
functions documented in a central point you refer to and/or documented
at the call site.

Anything that takes someone longer than a minute to work out is worth
half the minute writing the comment.


https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode498
> > lily/side-position-interface.cc:498: goto skip_skyline_stuff;
> > A goto.  Fabulous.
> >

> Are goto's really that evil when they're the easiest way to skip
> code?

The easiest way is to return the value you want to see returned.  It
would appear you have not even bothered looking at the target of the
goto, and yet you expect the reader of the code to search for the
target of the goto.  Why him and and not you?


https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode542
> > lily/side-position-interface.cc:542: Skyline_pair *v_orig =
> > Skyline_pair::unsmob (elt->get_property ("vertical-skylines"));
> > Is every element _guaranteed_ to have a property vertical-skylines?
Or
> > do we just crash if it doesn't?

> grob.cc line 83.

Fine, so you write

assert (v_orig);  // Should be assured by function Grob::whatever

here before using that value.  If someone changes grob.cc line 83 for
some reason, this assertion will make sure that the now-violated
assumptions will not go unnoticed.

> >
https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc#newcode301
> > lily/slur.cc:301: Interval yext = robust_relative_extent (script,
> > script, Y_AXIS);
> > What happens with cy?
> >

> We regain the effect of using cy by adding the offset via
> yextent.translate.

So why do we even calculate it?

I have not looked at the code changes so far; this is just a followup
on your comments.


https://codereview.appspot.com/7185044/