On 2013/04/28 21:59:37, janek wrote:
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.
I disagree with that reasoning since it is backwards. It's just simpler if Stencil and Box use the same criteria (actually, I wanted to throw out the check for the stencil expression being '() in Stencil::is_empty () as well, but that tripped up something in the page breaker and it looked like it would take a lot of time finding the cause). This simplicity was part of the reason to change Box's is_empty tests.
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? I am more worried that infinity_f might be defined with a different type/precision than that stored in Interval, making the comparison fail. It's not like the C++ compiler would generate a lot of code here, and I can probably even leave off the set_empty as it should be the default.
https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc File lily/stencil.cc (right):
https://codereview.appspot.com/8869044/diff/9001
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.
The current version has a bit more here.
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"?
Depends on the version. In the reviewed version, this is pretty accurate, but now I've changed behavior again. https://codereview.appspot.com/8869044/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
