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