On Oct 26, 2011, at 10:13 PM, carl.d.soren...@gmail.com wrote:
> I don't understand beam-quanting well enough to evaluate most of the
> code, but I've seen some concerns in properties and regtests.
>
> Thanks,
>
> Carl
>
>
Carl,
In general, you're absolutely right about the regtests. I've scrapped them all
and replaced them with the most pertinent extracts from the old regtest.
>
> http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc
> File lily/beam-quanting.cc (right):
>
> http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc#newcode411
> lily/beam-quanting.cc:411: init_instance_variables ();//if
> (!consistent_broken_slope) printf ("STATS0 EH %4.4f %4.4f X%4.4f Y %4.4f
> %4.4f\n", extremal_hangover_[LEFT], extremal_hangover_[RIGHT], x_span_,
> unquanted_y_[LEFT], unquanted_y_[RIGHT]);
> IMO, you should have a separate branch without the printf statements.
> We should not be asked to approve code containing commented out
> statements with the assurance that they will be removed before pushing.
> We should only be reviewing code intended for pushing (at least if this
> patch is intended for pushing as opposed to a design sketch).
>
> The printf statements, particularly in this format, make it very hard to
> read the code.
>
OK - removed.
> http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc#newcode1058
> lily/beam-quanting.cc:1058: /*
> Nice comment here. This is very helpful.
>
Thanks!
> http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc
> File lily/beam.cc (right):
>
> http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc#newcode979
> lily/beam.cc:979: bool consistent_broken_slope = broken_beam_style !=
> ly_symbol2scm ("individual");
> Here (and in scm/define-grobs.scm) you use "individual"; in
> scm/define-grob-properties.scm you use "separate".
>
Will change.
> And I see no code here for "peters".
Check Beam::quanting
>
> http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc#newcode1459
> lily/beam.cc:1459: "The property @code{broken-beam-style} can be
> @code{separate},"
> Values should not be listed here. They are listed in
> scm/define-grob-properties.scm. If they are listed in two places, they
> can get out of step and nobody will know which one is correct. IMO,
> these two lines should be completely removed. There is no need to list
> the property that is part of the interface here; it will be
> automatically generated by the docs.
>
OK - removed.
> http://codereview.appspot.com/5293060/diff/2004/lily/spanner.cc
> File lily/spanner.cc (right):
>
> http://codereview.appspot.com/5293060/diff/2004/lily/spanner.cc#newcode240
> lily/spanner.cc:240: Interval (1,-1));
> Some comments in here about the strategy to get the spanner length would
> probably be helpful. You have three different methods, IIUC, that are
> called depending on the situation.
>
Added.
> http://codereview.appspot.com/5293060/diff/2004/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
>
> http://codereview.appspot.com/5293060/diff/2004/scm/define-grobs.scm#newcode354
> scm/define-grobs.scm:354: (broken-beam-style . individual)
> This is not one of the alternatives listed in
> scm/define-grob-properties.scm
Naming fixed.
Your comments are really helpful - I don't expect people to know how
beam-quanting works (it took me a long long time to figure it out), but I think
I focused so much on the technical details of quanting that I let some basic
stuff (like consistency in naming) slip. I certainly appreciate your catching
it!
Cheers,
MS_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
