On 2012/09/12 11:48:45, mike7 wrote:
>
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode87
> lily/chord-name-engraver.cc:87: {
> On 2012/09/06 08:50:40, dak wrote:
>> What kind of contorted logic and guessing game is that?
>> if (make_markup)
>> {
>> [old code ending in setting "text"]
>> }
>
>> Please don't obfuscate code just to save reindentation.
>
> This comment has not been addressed.
>
I think I misunderstood your comment
I have a hard time finding an interpretation where doing nothing is
the proper remedy.
- could you show what you mean by proposing
an alternative (in pseudo-code if you'd like) to the code that's in
there?
I did it in pseudo-code above, and you ignored it. So let's do it in
straight code.
The diff to the original has the form
- if (rest_event_)
+ if (rest_event_ && !make_markup) { }
+ else if (rest_event_)
{
SCM no_chord_markup = get_property ("noChordSymbol");
if (!Text_interface::is_markup (no_chord_markup))
...
} else
when it should rather have the form
if (rest_event_)
{
- SCM no_chord_markup = get_property ("noChordSymbol");
- if (!Text_interface::is_markup (no_chord_markup))
...
+ if (make_markup)
+ {
+ SCM no_chord_markup = get_property ("noChordSymbol");
+ if (!Text_interface::is_markup (no_chord_markup))
...
+ }
} else
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode136
> lily/chord-name-engraver.cc:136: markup = scm_call_4 (name_proc,
> pitches, bass, inversion,
> On 2012/09/06 08:50:40, dak wrote:
>> You do all the calculation and then throw it away? Where is the
> point?
>
> This comment has not been addressed. If neither rest_event nor
> make_markup are set, you calculate pitches, bass, inversion and
throw
> them away.
I'm not sure what you mean - every time these are calculated, these
are used on
line 139.
Only if make_markup is set. Otherwise you calculate them and throw
them away.
Why? Why do all the work in the case where make_markup is not even
set? Why not skip all that much earlier when make_markup is not set?
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
> lily/chord-name-engraver.cc:149: && ly_is_equal (chord_as_scm,
> last_chord_))
> On 2012/09/06 09:59:09, MikeSol wrote:
>> On 2012/09/06 08:50:40, dak wrote:
>> > If one is doing the chord calculation manually, you can't make
the
> decision of
>> > whether a chord changed based on the automatic calculation. For
> better or
>> > worse, you need to compare the computed chord versions/text.
>
>> To respond to your points above, I don't throw away the values
above
> because
>> they're used here.
>
> I don't see where the current code version would use them. Can you
> point that out to me?
>
They're no longer used as per your suggestion to compare text
instead of these values.
And is there a reason the logic of the code is not changed to reflect
the changes of the code?
http://codereview.appspot.com/6496085/
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
