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

[dak]

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.


https://codereview.appspot.com/7185044/diff/17001/lily/directional-element-interface.cc
File lily/directional-element-interface.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/directional-element-interface.cc#newcode25
lily/directional-element-interface.cc:25: recursive_get_grob_direction
(Grob *me)
Why is not direction-source used instead?  This code seems like being
intended to replace proper information by heuristics that mask a problem
most of the time.

https://codereview.appspot.com/7185044/diff/17001/lily/script-interface.cc
File lily/script-interface.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/script-interface.cc#newcode81
lily/script-interface.cc:81: // ie DynamicText takes direction from
DynamicLineSpanner
Why then would DynamicLineSpanner not be the direction-source of
DynamicText?  I applaud that you are bucking the trend of making the
entire patch without comments, but it looks like the code is intended to
mask a bug by sometimes doing the right thing by accident.

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc
File lily/side-position-interface.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode185
lily/side-position-interface.cc:185: // or with anything in
other_v_skylines.
Congrats.  This is actually the first comment that actually gives _any_
information for the actual intent of a function.  Unfortunately, there
is no indication _what_ is being returned.
"avoid_outside_staff_collisions" does not sound like it would return any
value.  And what information do we get?  "Returns the value for the grob
elt" (there is no grob elt in the arguments) "whose skylines are given
by h_skyline..." (there is no h_skyline in the arguments) "so that it
doesn't intersect with staff_skyline" (there is no staff_skyline in the
arguments).

And what is the grob going to do with that value to stop intersecting?
Scale itself by that value?  Buy itself a hoverboard worth the value in
order to ride the skylines?

Please give the function a name making any sense in connection with its
return value, and if you bother writing a comment, please make sure that
it refers to actually involved variables and arguments.  And what's with
padding, horizon_padding, other_padding, and other_horizon_padding?
What's with dir?

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode296
lily/side-position-interface.cc:296: //printf ("        Q %s\n", e->name
().c_str ());
What's this supposed to be?

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?

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.

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.

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode505
lily/side-position-interface.cc:505: // ugh...gets called too often...?
This comment sounds like you had unexpected effects when debugging.  Are
you going to leave them in as easter eggs?

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode515
lily/side-position-interface.cc:515: if (Skyline_pair
*inside_staff_skylines = Skyline_pair::unsmob (vag->get_object
("inside-staff-skylines")))
get_object can _create_ an object via callback etc, and then the _only_
SCM referring to it is unsmobbed by you.  The Skyline_pair is
_immediately_ eligible for garbage collection for that reason.  You need
to assign the return value of get_object to an SCM variable.  And you
either unsmob it for every use, _or_ you need to use
scm_remember_upto_here_1 on it right behind the last use of the
unsmobbed Skyline_pair since otherwise the optimizer might optimize the
variable away and then the Scheme garbage collector does not see it is
still in use.

https://codereview.appspot.com/7185044/diff/17001/lily/side-position-interface.cc#newcode539
lily/side-position-interface.cc:539: for (; i < elements.size () &&
elements[i] != me; i++)
What's that condition?  You stop _completely_ when reaching me?  Do you
not rather mean to merely _skip_ that particular element?

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?

https://codereview.appspot.com/7185044/diff/17001/lily/slur.cc
File lily/slur.cc (right):

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?

https://codereview.appspot.com/7185044/diff/17001/lily/system.cc
File lily/system.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/system.cc#newcode423
lily/system.cc:423: Axis_group_interface::sort_outside_staff_elements
(me, vertical_skyline_grobs);
Which algorithm requires the outside_staff_elements to be sorted, where
is that requirement documented?

https://codereview.appspot.com/7185044/diff/17001/lily/tuplet-bracket.cc
File lily/tuplet-bracket.cc (right):

https://codereview.appspot.com/7185044/diff/17001/lily/tuplet-bracket.cc#newcode494
lily/tuplet-bracket.cc:494: my_offset is, for now, only used for
cross-staff grobs.
The code belies this statement.  In fact, my_offset has been _removed_
from the code conditioned on !cross-staff, and is used below in code not
depending either way on cross-staff.

So it would be quite important to document what it is really supposed to
be used for.

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