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/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
