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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel