Re: Macro for(UP_and_DOWN) and 3 similar. (issue 2491) (issue 6109046)

[milimetr88]

Reviewers: Keith, Graham Percival,


http://codereview.appspot.com/6109046/diff/1/flower/include/direction.hh
File flower/include/direction.hh (right):

http://codereview.appspot.com/6109046/diff/1/flower/include/direction.hh#newcode63
flower/include/direction.hh:63: // huh?
On 2012/04/24 03:32:28, Keith wrote:
> If you replace *all* the while(flip()) loops, you can remove the flip
function,
> which is merely object-oriented obfuscation for the unary minus
operator.

Well, there will be one more place: spacing-spanner.cc:291
There is:
  while (flip (&d) != LEFT && rb);

By now I can't tell what the whole code is for. Could you give me a tip?

http://codereview.appspot.com/6109046/diff/1/flower/include/direction.hh#newcode78
flower/include/direction.hh:78: #define DOWN_and_UP(d) \
On 2012/04/24 04:00:02, Graham Percival wrote:
> I see that our code uses both versions, and I fully support this patch
making a
> simple substitution for these macros.

> After it's accepted, it might be nice to investigate this further: do
we ever
> depend on the order of DOWN_and_UP, and if not, could we standardize
on
> UP_and_DOWN (or vice versa).

> It might also be nice to standardize on either (d) or (dir); I support
the
> latter.  However, this is again something which should happen after
this patch
> is pushed.

Yes - first let's push THAT patch and then discuss on other changes
linked to my macros :)

Btw as for (d) or (dir) almost always (d) was used in the code. (dir)
occured only sometimes. In some single cases there were other names used
such as: downup, event_dir, which or updowndir. But that's a topic for a
next patch :)

http://codereview.appspot.com/6109046/diff/1/lily/ledger-line-spanner.cc
File lily/ledger-line-spanner.cc (right):

http://codereview.appspot.com/6109046/diff/1/lily/ledger-line-spanner.cc#newcode52
lily/ledger-line-spanner.cc:52: + current_extents[d].length ();
On 2012/04/24 04:10:36, Keith wrote:
> On 2012/04/24 03:32:28, Keith wrote:
> > but it looks okay to me.
> I take that back.  There is exactly the problem that HanWen predicted
in the
> email chain. Chords in seconds get a bit too much space, if they have
ledgers.
> { \time 4/1
>    \override Score.SpacingSpanner #'packed-spacing = ##t
>    c'1 a <b'' c'''> <g''' a'''> }
> The cause seems to be the decision to scale the ledger from the
notecolumn
> width, so it seems to fix it would require a bit of a redesign.

> Certainly not related to the rest of this patch.

To be honest, by now I don't try to understand what Han Wenn and you are
trying to say about this bug - first I would like to finish the
for_UP_and_DOWN patch. Is that ok? :)
As I guess, this bug should be filed as a separate issue, right? Should
I ask such questions or just do what I think is proper?

http://codereview.appspot.com/6109046/diff/1/lily/ledger-line-spanner.cc#newcode68
lily/ledger-line-spanner.cc:68: while (flip (&d) != DOWN);
On 2012/04/24 03:32:28, Keith wrote:
> Possibly you have found the cause for
> <http://code.google.com/p/lilypond/issues/detail?id=2493>

> One nice thing about your macro (or a loop with both conditions in one
place) is
> that it helps to avoid this type of mistake.

Nice although that was of course unintentional :)

On 22 April 2012 13:28, Han-Wen Nienhuys <address@hidden> wrote:
Looks like a bug.  Maybe you could work on a test/repro case?

Well, my understanding of this code is really, really vague, so it would
take me a lot of time to: understand this, learn how to write test cases
and then write one.
Therefore I will just do what Keith suggested - I'll correct that in
this issue, but as a separate commit.

Description:
Macro for(UP_and_DOWN) and 3 similar.
Replaces do{ ... } while(flip (&d) != DOWN/UP/... with a macro
for(DOWN_and_UP/UP_and_DOWN/....) { }

http://code.google.com/p/lilypond/issues/detail?id=2491

Please review this at http://codereview.appspot.com/6109046/

Affected files:
  M flower/include/direction.hh
  M lily/ambitus-engraver.cc
  M lily/axis-group-interface.cc
  M lily/beam-quanting.cc
  M lily/beam.cc
  M lily/bezier.cc
  M lily/break-substitution.cc
  M lily/dynamic-align-engraver.cc
  M lily/figured-bass-continuation.cc
  M lily/hairpin.cc
  M lily/horizontal-bracket.cc
  M lily/interval-minefield.cc
  M lily/item.cc
  M lily/ledger-line-spanner.cc
  M lily/line-spanner.cc
  M lily/lookup.cc
  M lily/lyric-hyphen.cc
  M lily/multi-measure-rest-engraver.cc
  M lily/multi-measure-rest.cc
  M lily/new-fingering-engraver.cc
  M lily/note-collision.cc
  M lily/note-spacing.cc
  M lily/ottava-bracket.cc
  M lily/ottava-engraver.cc
  M lily/paper-column.cc
  M lily/piano-pedal-bracket.cc
  M lily/rest-collision.cc
  M lily/rod.cc
  M lily/script-column.cc
  M lily/slur-configuration.cc
  M lily/slur-scoring.cc
  M lily/slur.cc
  M lily/spacing-determine-loose-columns.cc
  M lily/spacing-interface.cc
  M lily/spanner.cc
  M lily/staff-symbol.cc
  M lily/stem.cc
  M lily/system-start-delimiter.cc
  M lily/system.cc
  M lily/tie-column.cc
  M lily/tie-engraver.cc
  M lily/tie-formatting-problem.cc
  M lily/tie.cc
  M lily/tuplet-bracket.cc