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

[dak]

I have a headache after the first file of 30, so this is just this one
file and does not imply that the others are fine.


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

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#oldcode1032
lily/axis-group-interface.cc:1032: "outside-staff-placement-directive "
There is probably a reason that this

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#oldcode1041
lily/axis-group-interface.cc:1041: "vertical-skyline-elements "
and this property have been removed from the Axis_group_interface even
though they are still being used.  What's the reason?

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

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode45
lily/axis-group-interface.cc:45: staff_priority_less (Grob *const &g1,
Grob *const &g2)
There is no point in passing references to pointers when the pointers
are not actually being changed.

Do you mean to pass references to the grobs instead?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode59
lily/axis-group-interface.cc:59: return g1 < g2;
Making decisions based on memory address is a bad idea since it leads to
irreproducible behavior.  Since the order does not need changing, one
can just return true here.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode77
lily/axis-group-interface.cc:77: static void
There is no comment that indicates what add_interior_skylines can be
used for, what its input and output are, where and when it should be
used.

This is write-only code.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode99
lily/axis-group-interface.cc:99: SCM
There is no comment what valid_outside_staff_placement_directive is
called for and what its results are.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode104
lily/axis-group-interface.cc:104: if ((directive == ly_symbol2scm
("left-to-right-greedy"))
If there is only a limited set of valid values, this should be checked
by an appropriate type definition of outside-staff-placement-directive
at assigment time rather than blowing up at run time.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode691
lily/axis-group-interface.cc:691: MAKE_SCHEME_CALLBACK
(Axis_group_interface, calc_inside_staff_skylines, 1);
There is no comment what calc_inside_staff_skylines is supposed to
calculate, what marks an "inside_staff_skyline", and how it differs from
other skylines.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode701
lily/axis-group-interface.cc:701: assert (y_common == me);
If skylines are disabled, why would this assertion be true?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode724
lily/axis-group-interface.cc:724:
sane_outside_staff_priority_parental_relationship (Grob *me)
What has this to do with sanity?  Are childs only allowed larger staff
priorities?  Why?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode739
lily/axis-group-interface.cc:739: ancestor_priority_plus_a_bit (Grob
*me)
Why do we need this?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode749
lily/axis-group-interface.cc:749: return scm_from_double (scm_to_double
(ancestor->get_property ("outside-staff-priority")) + 0.0001);
This does not distinguish between various ancestors, so both a child as
well as a grandchild will get assigned the same priority based on their
common ancestor.  If the degree of ancestry does not count into the
result, it would appear that this function only serves for
disambiguation at a single level.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode767
lily/axis-group-interface.cc:767: in two movings.
Huh?  Above we called a staff priority relation "sane" when the parent
had a smaller outside staff priority.  Now it must not have any outside
staff priority?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode771
lily/axis-group-interface.cc:771: elements[i]->warning ("An elements' Y
parent must have a lower outside staff priority than the element.");
Looks like the above comment got things wrong.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode778
lily/axis-group-interface.cc:778: are triggered beforehand.
But we do not _do_ any sorting here.  Why would we not call the extents
before we do the actual sorting rather than in an unrelated function?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode785
lily/axis-group-interface.cc:785: MAKE_SCHEME_CALLBACK
(Axis_group_interface, vertical_skyline_elements, 1);
There is no comment what this function returns, and it obviously does
not merely return the elements as its name would suggest.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode841
lily/axis-group-interface.cc:841: elt->programming_error ("Sorting
function should have made sure that I have an outside-staff-priority
although my parent does. Setting to parent's...");
The error message does not make sense. "Sorting function should have
made sure that I have an outside-staff-priority although my parent
does."?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode851
lily/axis-group-interface.cc:851: bool l2r = ((directive ==
ly_symbol2scm ("left-to-right-greedy"))
Actually, this strongly suggests that the "directive" should be split
into two properties, one being a Direction property, the other being a
boolean.  It makes no sense to conflate orthogonal properties into a
single property just to pick them apart afterwards.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode887
lily/axis-group-interface.cc:887: && scm_is_eq (elements[i +
1]->get_property ("outside-staff-priority"), priority))
Priorities are generally numeric and cannot reliably be compared with
scm_is_eq.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode899
lily/axis-group-interface.cc:899: for (vsize j = l2r ? 0 :
current_elts.size ();
This is rather inscrutable, and it is worth noting that this does not
work symmetrically since the sort order, as it has been established
here, is _always_ according to the _left_ edge of a grob.  So for l->r
placement, we are sorted according to trailing edge, for r->l placement,
according to the leading edge.

It also seems utterly pointless to go through the loop when polite ==
false.  One can just return the array, potentially reversed.

Since r->l order is simulated by going reverse anyway, one can just do
the conditional reverse independent of polite, return on !polite and
then do the loop in forward direction either way.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode917
lily/axis-group-interface.cc:917: if (x_extent[LEFT] <= last_end[dir] &&
polite)
Shouldn't that be < here?

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode927
lily/axis-group-interface.cc:927: skipped_elements.clear ();
If we have r->l order here, the loop was run backwards, pushing in this
backwards order into skipped_elements.  However, the next loop will
again be run backwards, meaning that in the case of r->l ordering,
skipped_elements will be ordered the wrong way round.

https://codereview.appspot.com/7185044/diff/164001/lily/axis-group-interface.cc#newcode970
lily/axis-group-interface.cc:970: if (!ancestor && !scm_is_number
(elt->get_property ("outside-staff-priority")))
There is no point in calculating the outside_staff_ancestor when this
element has an outside-staff-priority.

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