12 years, 11 months ago
(2012-04-25 06:29:14 UTC)
#2
http://codereview.appspot.com/5908043/diff/1/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/5908043/diff/1/lily/beam.cc#newcode1326
lily/beam.cc:1326: || beam->get_property_data ("direction") == ly_symbol2scm
("calculation-in-progress"))
On 2012/04/25 06:21:54, Keith wrote:
> Mike, any idea why you added this?
> It just adds to the cases where we lie and say the beam is on the centerline.
> It causes this to go wrongly
> << {b''8[ r16. g''32] } \\ {r8 <f' g' a' b'>16. r } >>
>
> Take a look at today's patchset at
> http://codereview.appspot.com/6035053/
If this bit isn't there, a circular dependency could be called up later on in
the function while looking for the direction. I forget the exact reason for the
circular dependency, but it crept up in several regtests. Like several other
pure functions, I have it cowardly fail if a pure estimation is not possible
because of circular dependencies, which usually has a minimal impact on the
typeset result.
12 years, 11 months ago
(2012-04-25 11:10:40 UTC)
#3
http://codereview.appspot.com/5908043/diff/1/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/5908043/diff/1/lily/beam.cc#newcode1326
lily/beam.cc:1326: || beam->get_property_data ("direction") == ly_symbol2scm
("calculation-in-progress"))
On 2012/04/25 06:29:14, mike7 wrote:
> On 2012/04/25 06:21:54, Keith wrote:
> > Mike, any idea why you added this?
> > It just adds to the cases where we lie and say the beam is on the
centerline.
> > It causes this to go wrongly
> > << {b''8[ r16. g''32] } \\ {r8 <f' g' a' b'>16. r } >>
> >
> > Take a look at today's patchset at
> > http://codereview.appspot.com/6035053/
>
> If this bit isn't there, a circular dependency could be called up later on in
> the function while looking for the direction. I forget the exact reason for
the
> circular dependency, but it crept up in several regtests. Like several other
> pure functions, I have it cowardly fail if a pure estimation is not possible
> because of circular dependencies, which usually has a minimal impact on the
> typeset result.
Isn't it a good idea to make a comment in the code to this part of if condition?
This would make that fragment of code clear for other developers.
12 years, 11 months ago
(2012-04-25 11:14:41 UTC)
#4
On 2012/04/25 11:10:40, Milimetr88 wrote:
> http://codereview.appspot.com/5908043/diff/1/lily/beam.cc
> File lily/beam.cc (right):
>
> http://codereview.appspot.com/5908043/diff/1/lily/beam.cc#newcode1326
> lily/beam.cc:1326: || beam->get_property_data ("direction") == ly_symbol2scm
> ("calculation-in-progress"))
> On 2012/04/25 06:29:14, mike7 wrote:
> > On 2012/04/25 06:21:54, Keith wrote:
> > > Mike, any idea why you added this?
> > > It just adds to the cases where we lie and say the beam is on the
> centerline.
> > > It causes this to go wrongly
> > > << {b''8[ r16. g''32] } \\ {r8 <f' g' a' b'>16. r } >>
> > >
> > > Take a look at today's patchset at
> > > http://codereview.appspot.com/6035053/
> >
> > If this bit isn't there, a circular dependency could be called up later on
in
> > the function while looking for the direction. I forget the exact reason for
> the
> > circular dependency, but it crept up in several regtests. Like several
other
> > pure functions, I have it cowardly fail if a pure estimation is not possible
> > because of circular dependencies, which usually has a minimal impact on the
> > typeset result.
>
I'm not well-placed to say. I try to let the code act as its own commentary as
much as possible. Here, I think that a person who knows the code base knows
that if the function exits because a property's calculation is in progress, it
is to avoid triggering that calculation. However, if anyone feels that this is
not pick-upable upon reading, please add the comment!
Issue 5908043: Brings beamed rest closer to cross staff beam.
(Closed)
Created 13 years ago by MikeSol
Modified 12 years, 7 months ago
Reviewers: Keith, mike7, Milimetr88
Base URL:
Comments: 3