Macro for(UP_and_DOWN) and 3 similar. (issue 2491)

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

[Keith, 2012-04-24 03:32:28]

Looks good to me, 
but I suggest you also solve the bug you found (issue 2493) in this patch but
preferably as a separate commit.
Then you can convert all the loops.

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#newc...
flower/include/direction.hh:63: // huh?
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.

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#newc...
lily/ledger-line-spanner.cc:52: + current_extents[d].length ();
This was mentioned as suspicious in the email, but it looks okay to me.

http://codereview.appspot.com/6109046/diff/1/lily/ledger-line-spanner.cc#newc...
lily/ledger-line-spanner.cc:68: while (flip (&d) != DOWN);
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.

[Graham Percival, 2012-04-24 04:00:02]

LGTM

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#newc...
flower/include/direction.hh:78: #define DOWN_and_UP(d) \
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.

[Keith, 2012-04-24 04:10:36]

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#newc...
lily/ledger-line-spanner.cc:52: + current_extents[d].length ();
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.

[Milimetr88, 2012-04-24 16:21:17]

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#newc...
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#newc...
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#newc...
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#newc...
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 <hanwenn@gmail.com> 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.

[dak, 2012-04-25 09:57:48]

It would appear that you ignored
<http://permalink.gmane.org/gmane.comp.gnu.lilypond.auto/465>, 
<http://permalink.gmane.org/gmane.comp.gnu.lilypond.auto/466>, the end of
<http://permalink.gmane.org/gmane.comp.gnu.lilypond.devel/46312> and
<http://permalink.gmane.org/gmane.comp.gnu.lilypond.auto/442>.

Code reviews take time, effort, and diligence.  Patch testing can be done by
yourself easily.  If it isn't, it again takes time, effort, and diligence.

It is a matter of courtesy not to waste those lightly, and treat the resources
of your coworkers with the respect you want to have them treat yours.

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

http://codereview.appspot.com/6109046/diff/9001/lily/ledger-line-spanner.cc#o...
lily/ledger-line-spanner.cc:68: while (flip (&d) != DOWN);
This was a _one_-iteration loop.  After the change, it is a _two_-iterations
loop.

http://codereview.appspot.com/6109046/diff/9001/lily/slur.cc
File lily/slur.cc (right):

http://codereview.appspot.com/6109046/diff/9001/lily/slur.cc#newcode105
lily/slur.cc:105: ret.add_point (d[dir]);
The loop does not appear to use the index downup.  Anybody have an idea what's
up with that?  This is a faithful reproduction of the original loop, but does
not appear to make sense.

[Milimetr88, 2012-04-25 11:44:35]

On 25 April 2012 11:57, <dak@gnu.org> wrote:

> It would appear that you ignored
>
<http://permalink.gmane.org/**gmane.comp.gnu.lilypond.auto/**465<http://permal...
> >,
>
<http://permalink.gmane.org/**gmane.comp.gnu.lilypond.auto/**466<http://permal...
> >,


No - just look at dates - the comments are newer than my third patch set.

the end

of
<http://permalink.gmane.org/**gmane.comp.gnu.lilypond.devel/**46312<http://per...>
>

I have even written an answer on Keith's mail, but by mistake sent it only
to him. I wrote there:

As I understand, fixxcc.py will correct that automatically, so I don't have
to bother about that? Nevertheless I'll remember that for my future patches.



>
<http://permalink.gmane.org/**gmane.comp.gnu.lilypond.auto/**442<http://permal...
> >.
>
I didn't notice that comment. I'm not used yet to checking comments on
Google Code.


> Code reviews take time, effort, and diligence.  Patch testing can be
> done by yourself easily.  If it isn't, it again takes time, effort, and
> diligence.
>
> It is a matter of courtesy not to waste those lightly, and treat the
> resources of your coworkers with the respect you want to have them treat
> yours.
>

For me it sounds like blaming me that I'm a beginner developer on Lilypond
project, so my work isn't as optimized as it should be. It's not nice for
me, really, and it doesn't encourage me to submit my patches either.

I'd like to know how to run regtests. Should I follow:
http://lilypond.org/doc/v2.15/Documentation/contributor/regtest-comparison and
compare all those tests that differ?


Łukasz

[Milimetr88, 2012-04-25 11:47:07]

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

http://codereview.appspot.com/6109046/diff/9001/lily/ledger-line-spanner.cc#o...
lily/ledger-line-spanner.cc:68: while (flip (&d) != DOWN);
On 2012/04/25 09:57:48, dak wrote:
> This was a _one_-iteration loop.  After the change, it is a _two_-iterations
> loop.

In http://permalink.gmane.org/gmane.comp.gnu.lilypond.devel/46279 Keith asked me
to change that in a separate commit, so I did so. Did he wanted me to do
something else?

[Milimetr88, 2012-04-25 11:49:02]

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

http://codereview.appspot.com/6109046/diff/9001/lily/ledger-line-spanner.cc#o...
lily/ledger-line-spanner.cc:68: while (flip (&d) != DOWN);
On 2012/04/25 11:47:07, Milimetr88 wrote:
> On 2012/04/25 09:57:48, dak wrote:
> > This was a _one_-iteration loop.  After the change, it is a _two_-iterations
> > loop.
> 
> In http://permalink.gmane.org/gmane.comp.gnu.lilypond.devel/46279 Keith asked
me
> to change that in a separate commit, so I did so. Did he wanted me to do
> something else?

Ah, sorry - a mistake. The third patch set was about something else. Ok, I'll
make a commit without correcting this loop.

[Milimetr88, 2012-04-25 13:14:54]

>
> I know that this is not a very positive message, but I am trying

 to save you yet more anguish.  The lilypond project currently does
> not even function smoothly between senior developers with more
> than 100 commits each; there is very little chance for a new
> contributor to have a smooth and pleasant time helping us.


Oh, I didn't know about that. I'm reading some Lilypond mails and they made
me the impression that you *urge* new contributors.

By the way, Janek stated in the mail regarding GSoC in February:

> Why is your organization applying to participate in Google Summer of
> Code 2012? What do you hope to gain by participating?
>    Most importantly: more contributors


I don't understand what's not working smoothly between senior developers.
Could you describe in a pair of sentences? Is it a situation without a
solution?

On 25 April 2012 15:02, Graham Percival <graham@percival-music.ca> wrote:

>
> >    For me it sounds like blaming me that I'm a beginner developer on
> >    Lilypond project, so my work isn't as optimized as it should be. It's
> >    not nice for me, really, and it doesn't encourage me to submit my
> >    patches either.
>
> *sigh*
> yes, this is a huge problem.  You really need a mentor to guide
> you through this process.  Janek Warcho did this for a few months,
> but he gave up due to university work.
>
> I believe that you're both Polish, so I really *really* wish that
> he would offer to guide you.  You're in the same time zone, you
> presumably speak the same native language, etc.


Yes, we are both Polish, yes, we are both in the same time zone, moreover
in the same city and we know each other. From time to time I call Janek and
he instructs me how to do this and that, but I thought it would be better
for lilypond to ask using an official channel - this list. In the present
situation, I'll talk with Janek and get to know if he could offer me more
help (yes, I know he is CC'd, but I'll talk him on my own).

Łukasz

[Keith, 2012-04-25 17:23:35]

http://codereview.appspot.com/6109046/diff/9001/lily/slur.cc
File lily/slur.cc (right):

http://codereview.appspot.com/6109046/diff/9001/lily/slur.cc#newcode105
lily/slur.cc:105: ret.add_point (d[dir]);
Here is the history:
http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode101

[Milimetr88, 2012-04-25 17:54:07]

http://codereview.appspot.com/6109046/diff/9001/lily/slur.cc
File lily/slur.cc (right):

http://codereview.appspot.com/6109046/diff/9001/lily/slur.cc#newcode105
lily/slur.cc:105: ret.add_point (d[dir]);
On 2012/04/25 17:23:35, Keith wrote:
> Here is the history:
> http://codereview.appspot.com/5431065/diff/8002/lily/slur.cc#newcode101

Or rather:
http://codereview.appspot.com/5431065/diff2/8002:9001/lily/slur.cc#newcode105

It seems that this for() loop is not necessary at all, I'll delete it *one day*
in a separate patch set (or a separate Google issue? give me a tip, please).

[Trevor Daniels, 2012-04-25 21:46:29]

Almost there, if the regtests run clean, but the added 'for' loops are still not
in GNU style - they need a space after 'for'.  This must be corrected before
pushing.  See
http://permalink.gmane.org/gmane.comp.gnu.lilypond.devel/46312 where Keith
explained this earlier.

[mike_apollinemike.com, 2012-04-26 06:55:27]

On 26 avr. 2012, at 07:28, Graham Percival wrote:

> 
> Well, right now we have nobody running the automated tests to
> check that new patches are ok.  So there will be no patches
> accepted to lilypond.
> 

I have a meeting in mid-May w/ the University of Paris VIII.  They're donating a
computer to LilyPond and I'll set patchy up on it.

Cheers,
MS

[Graham Percival, 2012-04-27 07:51:32]

LGTM