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