Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044)

[janek . lilypond]

On 2013/04/28 23:16:28, dak wrote:
> 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).

ok, and i agree that adding a comment like this into the source may not
be a good idea.  However, git commit message has a different purpose (to
myself at least); i want to know why something changed.  When i'm
searching lilypond history to learn something about the code i'm trying
to modify, my usual problem is that i find a change and the commit
message just says what changed - it doesn't answer my questions: "what
was wrong with previous version?  Was this just a cosmetic change that
accidentally introduced a bug, or was some other design idea behind it?"
I'm pretty surprised by your opinion here since you always advocate
writing hows and whys into commit messages.

> > 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.

ok.

https://codereview.appspot.com/8869044/