Re: flower: Get rid of libc-extension (issue 553740043 by address@hidden)

[jonas . hahnfeld]

On 2020/03/17 21:27:52, hanwenn wrote:
>
https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly
> File input/regression/tie-single-manual.ly (right):
> 
>
https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly#newcode19
> input/regression/tie-single-manual.ly:19: \override Tie.staff-position
= #-7.4
> On 2020/03/17 21:06:42, hahnjo wrote:
> > I'm not really happy about tweaking this test like this. It relies
on rounding
> > halfway always up (my_round) instead of away from zero (std::round).
I think
> > this is a coincidence, but if we want to retain this I need to go
through all
> > replacements of my_round -> std::round and check if the argument
could be
> > negative...
> 
> couldn't you inline my_round into the call site that needs this and
add a
> comment? The change otherwise risks breaking user tweaks, which we
should avoid
> if we can.

Yes, that was my thought. The problem is that I don't know in which call
sites could exhibit negative values, possibly all of them?

On 2020/03/17 21:28:35, lemzwerg wrote:
>
https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly
> File input/regression/tie-single-manual.ly (right):
> 
>
https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly#newcode19
> input/regression/tie-single-manual.ly:19: \override Tie.staff-position
= #-7.4
> > I'm not really happy about tweaking this test like this.
> > It relies on rounding halfway always up (my_round)
> > instead of away from zero (std::round). I think this is
> > a coincidence, but if we want to retain this I need to
> > go through all replacements of my_round -> std::round
> > and check if the argument could be negative...
> 
> Hmm.  Are you talking about rounding to integers?  If `staff-position`
is a
> floating point value, LilyPond takes it as an exact value, see
> `has_manual_delta_y`.  So there shouldn't be any rounding here...

Yes. I hope I've marked the correct rounding that I found out to be
responsible for the difference.


https://codereview.appspot.com/553740043/diff/547780043/lily/tie-formatting-problem.cc
File lily/tie-formatting-problem.cc (right):

https://codereview.appspot.com/553740043/diff/547780043/lily/tie-formatting-problem.cc#newcode908
lily/tie-formatting-problem.cc:908: conf.position_ = (int) std::round
(specifications_[i].manual_position_);
IIRC this is the call site that influences the regression test.

https://codereview.appspot.com/553740043/