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

[dak]

I've not really reviewed everything here, just highlights.  Regarding
the commenting problems: it is important for the reviewer or maintainer
or rereader to get the gist of the story.  Much of the LilyPond codebase
has been written with a total disregard to people not present during the
writing.  For that reason, it is not a useful metric for writing
LilyPond code that is supposed to get reviewed and maintained by people
other than the original author.

If a review is to make sense, "I think Han-Wen would likely understand
this" is not enough since the reviewers are different from Han-Wen, and
we want more people than just you and him to be able to maintain this
code in future.

The LilyPond code base has a maintainability problem due to historic
reasons, and we need to move away from that legacy rather than adding to
it.

Only part of this review is actually specific to this particular patch.
A lot is rather trying to bring across some general advice to coding and
commenting practice.


https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly
File input/regression/tuplet-number-outside-staff-positioning.ly
(right):

https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly#newcode15
input/regression/tuplet-number-outside-staff-positioning.ly:15:
\override TupletBracket.Y-offset =
#ly:side-position-interface::calc-outside-staff-y-offset
This is indicating a fundamental design problem: this override is not
related to the topic of the regtest.  If it is necessary for getting the
desired result, it will be necessary in a whole _lot_ of user-created
situations.  But it is not an easily user-accessibly override, and it is
not documented in normal user documentation.

This suspiciously looks like tampering with unrelated regtests in order
to mask fundamental problems from review.  Can you explain why this is
not the case?

https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly
File input/regression/tuplet-number-outside-staff-priority.ly (right):

https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly#newcode13
input/regression/tuplet-number-outside-staff-priority.ly:13: \override
TupletNumber.Y-offset =
#ly:side-position-interface::calc-outside-staff-y-offset
See above comment.  If outside-staff-priority ceases to work, a lot of
user-level documentation would warrant adaption.

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode595
lily/axis-group-interface.cc:595:
Axis_group_interface::staff_priority_less (Grob *const &g1, Grob *const
&g2)
Is there a reason you turn this into member functions?

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode713
lily/axis-group-interface.cc:713:
Axis_group_interface::prepare_for_outside_staff_calculations (Grob *me)
There is no description anywhere what this preparation will entail, what
it looks at, and what it does.  Is this used even more than once?

https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode755
lily/axis-group-interface.cc:755:
Axis_group_interface::sort_outside_staff_elements (Grob *me, vector<Grob
*> &elements)
This looks like it does a whole lot more than just sorting some
elements.  What does it do?  Why does it do that?

https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-interface.hh
File lily/include/axis-group-interface.hh (right):

https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-interface.hh#newcode69
lily/include/axis-group-interface.hh:69: static bool staff_priority_less
(Grob *const &g1, Grob *const &g2);
All those were previously staic functions inside of
axis-group-interface.cc, and thus limited to that file.  Now you have
made them part of the class definition, and in addition you have turned
then into a _public_ part of the class definition, suggesting that they
are part of the external interface of the class.

Why?

https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-element-interface.hh
File lily/include/directional-element-interface.hh (right):

https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-element-interface.hh#newcode26
lily/include/directional-element-interface.hh:26: // what is the
advantage not having these two as STATICs of GROB -- jcn
"these three" it would appear now.  And the question seems valid.

https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-interface.hh
File lily/include/side-position-interface.hh (right):

https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-interface.hh#newcode45
lily/include/side-position-interface.hh:45: static Real
calc_outside_staff_offset (Grob *me, bool pure, int start, int end, Real
current_offset);
Isn't that just something used internally by
calc_outside_staff_y_offset?  Is there a reason this is part of a
_public_ interface?

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

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode184
lily/side-position-interface.cc:184: // v_skyline) needs to move UP or
DOWN so that it doesn't intersect with
Presumably the function does not pick "UP or DOWN" on a whim, but it is
rather "in direction of dir".  However, if it can be _added_ to the
grob's Y-offset, it is _not_ the value to move UP or DOWN, but rather
the offset to add to Y-offset in order to position it beyond all
other_v_skylines with regard to direction dir (UP or DOWN).

Wait: you say "dir is the placement above or below the staff".  Does
that mean that dir can be UP, but the placement of the offset can be
_below_ the other_v_skylines but above the staff?  If so, will it first
attempt to fit elt _between_ staff and other_v_skylines, or will it go
outside in any case?

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode464
lily/side-position-interface.cc:464: Real outside_staff_offset = a ==
Y_AXIS
This is quite contorted and hard to read, what with = == ?:...

What you mean is
if (a == Y_AXIS)
  return scm_from_double (total_off
                          + calc_outside ...);
return scm_from_double (total_off);

If you want to, with an else.  Depends on your style.  Or the other way
round, starting with if (a == X_AXIS)...

But the point is that it is pointless to juggle with ?: to avoid an "if"
if one branch of ?: is going to "add" 0, namely do nothing anyway.  This
means not just different offsets, but different _actions_.  ?: can
always be replaced, but it has a few convenient use cases.  This isn't
one.  Code should not be an intelligence test, but boring.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode497
lily/side-position-interface.cc:497: // start: start of pure calculation
for spanners
Well, you are right that this place is strange to do a full
documentation of pure function arguments.  But we need to get one
eventually somewhere (not necessarily as part of this issue), and we are
talking about how to do proper commenting, so let's focus on what's
wrong about these comments anyway, even though they don't necessarily
belong here exactly.

Let's take apart what is wrong in detail:

They don't add useful information.

Well, apart from "for now facultative", but that's information about the
implementation rather than the use.

eventually "is this pure": it does not specify the target for
"eventually", and it does not specify "this".

What presumably is meant is "is the returned offset to be derived via
pure calculation?".

Then start and end are presumably _only_ relevant when it _is_ a pure
calculation.  Not mentioned.  Then the comment "start/end of pure
calculation for spanners" is totally awful.  The parameters are already
_called_ start/end, only repeating that is not helpful.  It is not at
all apparent what the connection with _spanners_ would be.  And "start
of pure calculation" is nondescriptive.  What is the unit that the
"start" is specified in?  Is it seconds since Jan 1 1970 UTC?  Or is it
rather some index into some data structure?  Which one?  Give a term
that can be reasonably found using "git grep", and "start" and "end"
alone are not really helpful for that.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode538
lily/side-position-interface.cc:538: if (Skyline_pair
*inside_staff_skylines = Skyline_pair::unsmob (iss))
After this line, GUILE no longer sees iss as something it needs to
protect from garbage collection.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode539
lily/side-position-interface.cc:539: skylines = Skyline_pair
(*inside_staff_skylines);
Now if Skyline_pair contains any elements allocated by GUILE
(non-immediate Scheme values), it will ask GUILE for new elements, and
GUILE might allocate them from *inside_staff_skylines data.  It turns
out from looking through the headers that Skyline_pair is free from SCM
data.  But doing that kind of research every time one reads the code is
costly, so it is easier if you just stick with the SCM value until you
have created your copy or finished with the whole thing.

unsmob it for every use until then, and GUILE will let it stick around
until the last unsmob.  Or put an scm_remember_upto_here_1 on the SCM
value after the last use of an unsmobbed pointer from it.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode565
lily/side-position-interface.cc:565: Skyline_pair *v_orig =
Skyline_pair::unsmob (elt->get_property ("vertical-skylines"));
Same here.

https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode593
lily/side-position-interface.cc:593: Skyline_pair v_skylines (*v_orig);
Too much distance to above, and several calls of get_property,
potentially allocating memory, in between.  I explained all this in the
previous review.  Please generalize one explanation to all occurences of
the same problem.  If the problem is not clear to you, ask questions
until it is.  Reviews (and things like valgrind) are a safety net, not a
trampoline.

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