Re: Potential fix for issue 1504. (issue4237057)

[mtsolo]

Thanks for the comments!
New patch uploaded.


http://codereview.appspot.com/4237057/diff/1/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode173
lily/beam.cc:173: span_data.push_back (get_beam_span
(orig->broken_intos_[i], commonx));
On 2011/03/09 23:03:44, hanwenn wrote:
> this looks wrong - different broken pieces cannot ever share a
commonx.
Fixed.

http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode620
lily/beam.cc:620: {
On 2011/03/11 14:01:29, hanwenn wrote:
> it would be nice if you could collapse all of the if (feather) code.
You're
> duplicating an awful lot of logic.

I'm not exactly sure what you mean here by "collapse."  I folded the
code that you mention below, but I'm not sure where it'd be possible to
reduce this code.

http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode647
lily/beam.cc:647: }
On 2011/03/11 14:01:29, hanwenn wrote:
> I see dup code.  Can you fold?

Done.

http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode653
lily/beam.cc:653: : segments[segments.size () - 1].vertical_count_);
On 2011/03/11 14:01:29, hanwenn wrote:
> use segments.back()

Done.

http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode665
lily/beam.cc:665: * (placements[RIGHT] - placements[LEFT])
On 2011/03/11 14:01:29, hanwenn wrote:
> placements.delta (), placements.length() ?

Done.

http://codereview.appspot.com/4237057/diff/1/lily/beam.cc#newcode682
lily/beam.cc:682: weights[LEFT] = 1;
On 2011/03/11 14:01:29, hanwenn wrote:
> can you set multiplier to 1 in this case?

Done.

http://codereview.appspot.com/4237057/diff/1/lily/spanner.cc
File lily/spanner.cc (right):

http://codereview.appspot.com/4237057/diff/1/lily/spanner.cc#newcode405
lily/spanner.cc:405: Spanner::broken_spanner_index (Spanner const *sp)
On 2011/03/09 23:03:44, hanwenn wrote:
> why not make it a real member funtcion?

Actually - sorry, I spoke too soon.  I see that there's a function
get_break_index.  Could these two functions be merged?  Is there a
reason that the two functions exist separately?  If not, I'll merge them
together.

http://codereview.appspot.com/4237057/