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

[address@hidden]

On 3 févr. 2013, at 21:33, address@hidden wrote:

> 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?

Instead of calling Grob::translate_axis.

> 
>> 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,

Yes, it assumes that if grob foo has no direction specified and its parent has 
one, then its direction is that of the parent.  In music, that makes sense for 
me.  Can you think of an outside-staff grob that has a Y child whose direction 
is not that of the Y parent.

> and that a child is always placed further from the axis
> than its parent.

That is not the logic implemented in the code.

> 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.

Fixed.

> 
> 
> 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.

grob.cc, line 411.
But where would you want the central explanation.

> 
> 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.

Where?

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

I'm totally cool with this, but where should I put it?

> 
> 
> 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?

Fixed.

> 
> 
> 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.

Fixed.

> 
>> >
> 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?

It's used elsewhere in the function.

Thanks for the review!

Cheers,
MS

> 
> I have not looked at the code changes so far; this is just a followup
> on your comments.
> 
> 
> https://codereview.appspot.com/7185044/