changes i see generally LGTM (but i may be missing something, in particular Keith seems to make a good point).
Let Stencil::is_empty use Box::is_empty [...]
I think the desription of this commit could use some more detail, for example that we need (1 . -1) stencils for backspacing.
Let Stencil::add_at_edge skip offsets for half-empty stencils [...]
I think "half-empty" is confusing (i at least was confused). I think "empty along only one axis" would be clearer.
Let Stencil::align_to check axis-specific emptiness before punting
could you use a more odrinary word? I had to check 'punt' in a dictionary, and i consider my English to be fairly good. https://codereview.appspot.com/8869044/diff/9001/lily/box.cc File lily/box.cc (right): https://codereview.appspot.com/8869044/diff/9001/lily/box.cc#newcode66 lily/box.cc:66: return interval_a_[a][LEFT] == empty[LEFT] Do we need to define 'empty'? I.e. why not just write interval_a_[a][LEFT] == infinity_f ? Are we expecting that the definition of empty extent will change further? https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc File lily/stencil.cc (right): https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode253 lily/stencil.cc:253: whitespace error https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode259 lily/stencil.cc:259: offset += d * padding; I think this is worth a comment with an example. It's not obvious why sometimes we want padding and sometimes not. https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm File scm/markup.scm (right): https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm#newcode67 scm/markup.scm:67: between the end of each stencil and the reference point of the following Hmm, shouldn't this read "between the end of the stencil and the *beginning* of the following stencil"? https://codereview.appspot.com/8869044/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
