https://codereview.appspot.com/7310075/diff/12001/lily/separation-item.cc File lily/separation-item.cc (right):
https://codereview.appspot.com/7310075/diff/12001/lily/separation-item.cc#newcode156 lily/separation-item.cc:156: // If these two uses of inf combine, leave the empty extent. The description is inaccurate since "combine" suggests a symmetry. Instead, the existing infinity trumps extra_width, regardless of which extent is infinite and which is empty. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode90 lily/skyline.cc:90: fatten_skinny_buildings (Real start, Real end) The function does nothing whatsoever to buildings. It works on intervals or ranges. So it is named misleadingly. In addition, it also "fattens" empty or undefined intervals (which have length 0). The "widening" is symmetric, meaning that a building at the left or the right of a skyline causes the skyline to expand. What is the point? Except for back-and-forth rotations, there is no operation that will cause an interval to move from non-empty to empty. The worst that can happen is that a non-zero interval collapses to a single point, but single points are treated just like non-zero intervals anyway. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode359 lily/skyline.cc:359: ret->push_back (Building (-infinity_f, -infinity_f, Three times -infinity_f? Looks fishy. If intentional, a comment is missing. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode499 lily/skyline.cc:499: given a minimum fatness of EPS. This comment has nothing whatsoever to do with the actions of this function. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode522 lily/skyline.cc:522: given a minimum fatness of EPS. Again, this comment has nothing to do with the actions of the function. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode617 lily/skyline.cc:617: /* do the same filtering as in Skyline (vector<Box> const&, etc.) */ Is this a hide-and-seek game? I don't see what kind of filtering Skyline (vector<Box> const&, etc.) does. In fact, it does something different than what you do now. https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode617 lily/skyline.cc:617: /* do the same filtering as in Skyline (vector<Box> const&, etc.) */ This is no longer the same filtering as in Skyline (vector<Box>... https://codereview.appspot.com/7310075/diff/12001/lily/skyline.cc#newcode689 lily/skyline.cc:689: warning ("Skyline horizon padding is too small. Widening to avoid numerical errors."); What numerical error are you thinking of here? With what consequences? https://codereview.appspot.com/7310075/diff/12001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/7310075/diff/12001/ly/engraver-init.ly#newcode896 ly/engraver-init.ly:896: \override Clef.Y-extent = #pure-safe-stencil-height What's pure-safe? uncached? Or what? https://codereview.appspot.com/7310075/diff/12001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/7310075/diff/12001/scm/define-grobs.scm#newcode1785 scm/define-grobs.scm:1785: (Y-extent . ,(ly:make-unpure-pure-container #f small-empty-interval)) What's that? No information for pure, a "small-empty-interval" for unpure? What happened to point intervals? https://codereview.appspot.com/7310075/diff/12001/scm/lily-library.scm File scm/lily-library.scm (right): https://codereview.appspot.com/7310075/diff/12001/scm/lily-library.scm#newcode646 scm/lily-library.scm:646: (define-public small-empty-interval '(0.01 . -0.01)) In the absence of units, this is very fuzzy. It also looks like a huge kludge waiting for problems: the interval is just as empty as the empty interval. Its only use is that it is less likely to make code blow up that can't deal with empty intervals but should. So the only purpose of this thing is to make it harder to fix bugs. https://codereview.appspot.com/7310075/diff/12001/scm/output-lib.scm File scm/output-lib.scm (right): https://codereview.appspot.com/7310075/diff/12001/scm/output-lib.scm#newcode61 scm/output-lib.scm:61: (define-public pure-safe-stencil-height We had the discussion about this already. What makes it "pure-safe"? The name is non-descriptive. Why would "pure" be "unsafe"? There is no comment explaining what you mean, and the naming is not giving any useful information. The information content of your identifiers is that of disassembled code, except that one tends to add comments to disassembled code. https://codereview.appspot.com/7310075/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel