Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)

[dak]

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/