100+ warnings

My main concern was fixing the warnings (~150, mostly vsize vs. int), and getting to know the codebase a bit.
  It consists mainly of int casts, with a few return value and syntax changes, and unused variable name deletions.
  Nothing serious.
  Make check gave no warnings, but please check it also.
I will post the second set of warning patches, once this was accepted (It's ready, I just don't want the patches to pile up)

[Carl, 2010-06-16 21:25:31]

Wow -- what a lot of code janitoring!  Thanks for the effort!

I'm not the person who will make the decision, but I'm a bit uncomfortable about
making this all in one huge patch.  If there are regressions, it'll be very hard
to track it down.  I'd feel better if it were split into several smaller patches
that affected specific functionality.  But I'm not sure that my unease is worth
having you do the work to do that.

You seem to prefer conversion to type casting, i.e. you prefer int(v_size) to
(int) v_size.  Is there a reason for this?

If you do want to use int(v_size), you should be aware that the code standards
of this project require a space before the parentheses, i.e. int (v_size), not
int(v_size).

As far as wrapping goes, the standards say that where possible we should wrap at
80 or less.  I suppose that's a decision we could revisit on -devel.

I have not reviewed all of the files in detail.  I'll try to get to it soon, but
it takes a long time to review 2700 lines and 99 files, even with a nice
side-by-side view.

Thanks,

Carl

[paPus, 2010-06-16 21:49:33]

I would have split it into chunks, but my main concern was warnings, which was
present in most of the files. Most of the committed files only contain casts.
Reviewing will take about 1-2 hours, I guess.

I have checked in the code how casting works and I have used the int(), mostly ,
without the space (could have used int() or static_cast<int>(), but this was the
most aesthetic to me). Must have found a wrong example. Should I correct it?
I don't really like the extra space decision, this way "while ()", "if ()" and
"clear ()" all look the same, while only the last one is a function, the rest
are not. And it even occupies too much space, but if that's the standard, that's
how I'll do it :). (Eclipse has a pretty good code formatter, someone might want
to run it (or maybe http://astyle.sourceforge.net/, haven't tried it, though) on
the whole project to beautify the code automagically)

Thanks,
  Lőrinc

[c_sorensen, 2010-06-17 00:51:08]

On 6/16/10 3:49 PM, "papuska@gmail.com" <papuska@gmail.com> wrote:

> Reviewers: carl.d.sorensen_gmail.com,
> 
> Message:
> I would have split it into chunks, but my main concern was warnings,
> which was present in most of the files. Most of the committed files only
> contain casts.
> Reviewing will take about 1-2 hours, I guess.
> 
> I have checked in the code how casting works and I have used the int(),
> mostly , without the space (could have used int() or static_cast<int>(),
> but this was the most aesthetic to me). Must have found a wrong example.
> Should I correct it?

Stroustrup mentions that you can do cast as

(type-name) expression

or

simple-type-name ( expression-list )

I prefer to always use

(type-name) expression

because the same syntax applies to both simple types and other types.

I think that's what I have mostly seen in the lilypond codebase.  But as far
as I know, this isn't a standard.


> I don't really like the extra space decision, this way "while ()", "if
> ()" and "clear ()" all look the same, while only the last one is a
> function, the rest are not. And it even occupies too much space, but if
> that's the standard, that's how I'll do it :).

It is the standard.  And submitting a patch is not the time to change the
standard.

> (Eclipse has a pretty
> good code formatter, someone might want to run it (or maybe
> http://astyle.sourceforge.net/, haven't tried it, though) on the whole
> project to beautify the code automagically)

We've had discussions about various formatters.  You can look for them on
-devel.  And in fact, there is a lilypond code formatter.

(see http://lilypond.org/doc/v2.13/Documentation/contributor/indentation)

However,  the diffs are so large that we are unwilling to run everything
through it.

See http://thread.gmane.org/gmane.comp.gnu.lilypond.devel/25397

The current recommendation for *indentation* is to use emacs, as shown in
the CG reference above.

Thanks,

Carl

[paPus, 2010-06-17 02:10:29]

> I prefer to always use (type-name) expression because the same syntax applies
to both simple types and other types.
 - yes, but you will have to parenthesize some expressions this way. int (a - b)
will become (int) (a - b), that is why I chose the first one, whenever I could.
static_cast<int>() would have solved both our problems, creating a new one,
though: it's freakin' ugly!

> It is the standard. And submitting a patch is not the time to change the
standard.
 - okay, I already said, that I'll comply, if I have to. I have just checked,
there are 245 "int("-s, would you like me to correct all of them and add it to
the patch? I can also correct just my patch, but why would I leave the others
there, if it's a mistake? It would increase the patch size, though.
I don't like disorder, but that is what I see in the code, it doesn't give a
pleasing look. Would you like me to test the Eclipse code formatter and sum it's
behavior up for you?

> We've had discussions about various formatters. You can look for them on
-devel.  And in fact, there is a lilypond code formatter.
> (see http://lilypond.org/doc/v2.13/Documentation/contributor/indentation)
 - Thanks, I have read everything, but I didn't like it. Telling people to use
Linux is okayish, because the used libs favor it, but telling them to use Emacs
sounds very unattractive (even for code style).
It's not easy being new here ...

Please let me know what changes you would like me to make, and I will.

Lőrinc

ps. why are many macros (MAKE_SCHEME_CALLBACK, ADD_INTERFACE etc) ended with a
';'?

[c_sorensen, 2010-06-17 03:32:30]

On 6/16/10 8:10 PM, "papuska@gmail.com" <papuska@gmail.com> wrote:

>> I prefer to always use (type-name) expression because the same syntax
> applies to both simple types and other types.
>   - yes, but you will have to parenthesize some expressions this way. int
> (a - b) will become (int) (a - b), that is why I chose the first one,
> whenever I could. static_cast<int>() would have solved both our
> problems, creating a new one, though: it's freakin' ugly!

Amen to that!

> 
>> It is the standard. And submitting a patch is not the time to change
> the standard.
>   - okay, I already said, that I'll comply, if I have to. I have just
> checked, there are 245 "int("-s, would you like me to correct all of
> them and add it to the patch?

Not at this time.  Somebody else might disagree with me.
 
> I can also correct just my patch, but why
> would I leave the others there, if it's a mistake? It would increase the
> patch size, though.

Wait for other people's comments.  I could be entirely wrong in my
assessment.

> I don't like disorder, but that is what I see in the code, it doesn't
> give a pleasing look. Would you like me to test the Eclipse code
> formatter and sum it's behavior up for you?

The issue is not "sum up its behavior".  Right now the issue is "can we get
it to be like emacs."

If you don't like the way emacs does it, then we have to have a battle about
changing the style rules.  At that point, instead of showing how Eclipse
does it, you'll want to show how the Eclipse way is better than the emacs
way.

Also, I expect that there will be resistance to Eclipse being used on a
philosophical basis, since LilyPond is a GNU project.  But don't take my
word for it.



> 
>> We've had discussions about various formatters. You can look for them
> on -devel.  And in fact, there is a lilypond code formatter.
>> (see
> http://lilypond.org/doc/v2.13/Documentation/contributor/indentation)
>   - Thanks, I have read everything, but I didn't like it. Telling people
> to use Linux is okayish, because the used libs favor it, but telling
> them to use Emacs sounds very unattractive (even for code style).
> It's not easy being new here ...
>

That's true.  My best recommendation is that you come and participate, and
then after you've done some good work (like your code janitor work, which
appears to me to be excellent, as far as I've been able to determine), you
can make a proposal that has a chance of being accepted.
 
> Please let me know what changes you would like me to make, and I will.

As I said above, wait for more feedback.
> 
> Lőrinc
> 
> ps. why are many macros (MAKE_SCHEME_CALLBACK, ADD_INTERFACE etc) ended
> with a ';'?

I don't know.  Ask on -devel.

Thanks,

Carl

[hanwenn, 2010-06-17 03:34:37]

* can you split this up by type of change? ie. one commit doing cosmetics of
comments, one changing inline functions, one for casts etc.  This makes
reviewing easier: a commit touching only comments can be approved with much less
scrutiny than one which changes logic.

* in case you are doing cosmetic changes, can you double check that your
standards are part of the style guide - I recall it is not that strict in many
areas, and in many cases there is no real reason to choose one over the other. 

* If there is no style guide standard, can you document the style first?  

* Why are all the casts there?  Is this a 64 bit compiler thing?  Lily compiles
virutally without warnings over here (core duo, gcc 4.4.4).  I think all the
casting hinders readability, so I propose to not add casts unless necessary.  If
the warnings bother you, add a targeted -Wno-xxx option to the Makefile.  I
doubt that there are any cases where there is the risk of a real error.

* The unsigned vsize is something we did after moving from our specialized
template to stl vectors (which apparently return size_t for some methods), and
in retrospect, I think it was not a wise change, as it was a lot of work, has
bought us little and introduces all kinds of bogus warnings about signed
comparisons, and now -with this patch- introduces lots more casting.  It is
probably an easier fix to change vsize to be an int again.

* Where do you get the 5-30% number? Last time I ran a profile, time was
dominated by get_property() and GC.  I am all for making things faster, but
measure before optimizing.

http://codereview.appspot.com/1724041/diff/1/59
File lily/note-head.cc (right):

http://codereview.appspot.com/1724041/diff/1/59#newcode124
lily/note-head.cc:124: Box b = fm->get_indexed_char_dimensions (k);
you've droppped the check here - are you sure about this? 
this is not a cosmetic change so it should be separate.

[hanwenn, 2010-06-17 03:39:12]

On 2010/06/17 02:10:29, paPus wrote:
> ps. why are many macros (MAKE_SCHEME_CALLBACK, ADD_INTERFACE etc) ended with a
> ';'?

This is so the emacs indenter does not get confused, mostly.

[paPus, 2010-06-17 10:56:05]

> * can you split this up by type of change? ie. one commit doing cosmetics of
> comments, one changing inline functions, one for casts etc.  This makes
> reviewing easier: a commit touching only comments can be approved with much
less
> scrutiny than one which changes logic.
 - I can, if I have to, but more than 90% are unsigned to signed casts or
changes. Almost all the comments cosmetics have just a space or two changed, If
I have to commit separately, I will drop those. There are only 3 inline function
changes, not worth a separate patch in my opinion. But again, I will do it, if
my code reviewers want it (do they?).

> * in case you are doing cosmetic changes, can you double check that your
> standards are part of the style guide - I recall it is not that strict in many
> areas, and in many cases there is no real reason to choose one over the other.
> * If there is no style guide standard, can you document the style first?
 - ouch ... I've just changed appearances that looked random or unnecessary
(like wrappings at ~60), or one variable parenthesized returns etc. Should I
drop all cosmetics and leave it a strict warning correction? The code won't
evolve this way, if -though being open source- everyone wants their code to be
untouched.

> * Why are all the casts there?  Is this a 64 bit compiler thing?  Lily
compiles
> virutally without warnings over here (core duo, gcc 4.4.4).  I think all the
> casting hinders readability, so I propose to not add casts unless necessary. 
If
> the warnings bother you, add a targeted -Wno-xxx option to the Makefile.  I
> doubt that there are any cases where there is the risk of a real error.
 - this junks my whole patch. Many casts were there in the first place, just in
the wrong place. Ignoring warnings is not very wise, in my opinion.
I totally agree that this is mostly just wrinkle lift, a proper correction would
be to only use signed variables (eg. first_page_num (and co.) is used as signed
and unsigned randomly).
Yes, it hinders readability. My first glance of the code suggests me (blindly)
that the long long and double are overused (I am probably wrong here).
The warnings should appear to you if you do a clean and rebuild, but yes, I am
using 64 bit everything.

> probably an easier fix to change vsize to be an int again.
 - well, that way the vector's length() (among others) has to be casted to int,
or the vector should be changed, but the whole STL depends on unsigned
variables, therefore with a custom vector class (which wouldn't be hard or a bad
idea) we couldn't use much of the STL (I'm guessing, haven't checked how much it
IS used in the code).
Considering the new TR1 changes of C++0x would also be wise, though maybe
premature. A programmer shouldn't get used to anything :).

> * Where do you get the 5-30% number? Last time I ran a profile, time was
 - sorry if I was misunderstandable, I only meant that those 2 functions were ~4
and ~26 percent faster in my little test program I wrote, than their branching
predecessors.

> you've droppped the check here - are you sure about this? 
> this is not a cosmetic change so it should be separate.
 - it *is* cosmetics, it's just that the unmarked casting to int made it
seemingly possible to be < 0, which it cannot be, since fm->name_to_index (key)
returns an unsigned positive value anyway (and it doesn't even depend on the
key, which is quite strange to me).
size_t
Font_metric::name_to_index (string) const
{
  return (size_t)-1;
}

Lőrinc

ps. added -devel to CC

[paPus, 2010-06-17 14:56:32]

> I ask that you submit the changes in a separated way: it's easier to review,
and if it does break something, it's easier to identify and revert the
incriminating commit.
 - Okay, I will resubmit some non-controversial parts of it in the near future.

> but just removing the check is not the right solution
 - I know this can happen in theory, but I was referring to the actual
implementation. I will leave the warning here.

I am more familiar with vimdoze and SVN, than linux and git, so thanks for
clarifying.

> Did you run the regression test?
 - I just did (make check?), it fails (I guess, I'm not sure what to look for).

> If we are doing a fixup of this, we should try to do all files at the same
time, or at least a section of files.
 - I will gladly do that, even if it takes a lot of time, but we should clarify
a few things first, as my developing RSI doesn't permit me to do things that
will simply be rejected (I do have some time now, however).
Should I write a basic coding guideline, based on the source code (and the
provided links), that you guys can modify so that everything important is
clarified (eg. spreadsheets.google.com)? A huge poll could follow it, with the
alternatives that were presented there.

> If we decide on a line length limit (I am fine with 100 or 80. We have 80 at
google, and I find it on the short side), we should document that and then fix
all instances of it in a structured way.
 - okay, then I guess I shouldn't post that patch yet, until it's decided.

> I doubt you can measure a speed difference between double and float.
 - I'm not certain here, but more floats can fit in the cache and if the
compiler does some vectorization or other SSE optimization, then twice as many
floats can be calculated in the same time.  Might gain speed if memory bandwidth
is the problem, but as far as I have read, it's rarely a bottleneck (you still
have to cast it, if you use both).
Just wrote a simple test case, the result were 3.21s for float and 3.79s for
double (15% diff, not much in this case).

> What is 'too many' ? Which macros could we do without?
 - I meant inline functions are many times preferable instead of preprocessor
macros.

> I agree with this in some cases but not in all, which is also why I want to
see more targeted commits, so we discuss specific cases.
 - Would it be possible, to discuss it first? I don't like to work in vain.

> It would be really nice to standardize on things, but it's a non-trivial task.
 - As a new member I find it hard to read a code that has the personality of all
its commiters, standardizing would really help, in case you want others to join
(otherwise it's okay, if you consider it to be).

Thanks,
    Lőrinc

[Graham Percival, 2010-06-17 16:16:04]

On Thu, Jun 17, 2010 at 3:56 PM,  <papuska@gmail.com> wrote:
>> Did you run the regression test?
>
>  - I just did (make check?), it fails (I guess, I'm not sure what to
> look for).

This is absolutely critical for any large-scale code change.


>> If we are doing a fixup of this, we should try to do all files at the
>
> same time, or at least a section of files.
>  - I will gladly do that, even if it takes a lot of time, but we should
> clarify a few things first, as my developing RSI doesn't permit me to do
> things that will simply be rejected (I do have some time now, however).
> Should I write a basic coding guideline, based on the source code (and
> the provided links), that you guys can modify so that everything
> important is clarified (eg. spreadsheets.google.com)? A huge poll could
> follow it, with the alternatives that were presented there.

Read the Contributor's Guide, and all the emails linked from:
http://code.google.com/p/lilypond/issues/detail?id=746

I'm not certain if you want to spend 20+ hours working on the code
style.  My recommendation is to work on non-contentious issues, like
fixing compiler warnings such as casting from signed to unsigned.


>> I agree with this in some cases but not in all, which is also why I
>
> want to see more targeted commits, so we discuss specific cases.
>  - Would it be possible, to discuss it first? I don't like to work in
> vain.

That's why it's vital to submit small patches, covering separate
matters -- yes, this makes it easier for us to discuss, but it also
protects you from doing a lot of work that later gets rejected.

Cheers,
- Graham

[paplorinc_yahoo.com, 2010-06-18 23:04:24]

> >  - I just did (make check?), it fails (I guess, I'm not sure 
what to look for).
 > This is absolutely critical for any large-scale code change.

 - I have checked out a clean clone of 
lily and after running

./autogen.sh
make all
make check
(and importing it into Eclipse which doesn't change anything, lily runs nicely)
it gives me this error:

[...]
ake[2]: Leaving directory `/home/papus/lilypond/input/regression/abc2ly'
rsync -L -a --exclude 'out-*' --exclude 'out' --exclude mf --exclude source
--exclude mf /home/papus/lilypond/out/share ./out-test
make[1]: Leaving directory `/home/papus/lilypond/input/regression/abc2ly'
rm -rf /home/papus/lilypond/out/test-results
mkdir -p /home/papus/lilypond/out/test-results
/home/papus/lilypond/scripts/build/out/output-distance --create-images
--output-dir /home/papus/lilypond/out/test-results
input/regression/out-test-baseline input/regression/out-test/
Traceback (most recent call last):
  File "/home/papus/lilypond/scripts/build/out/output-distance", line 1261, in
<module>
    main()
  File "/home/papus/lilypond/scripts/build/out/output-distance", line 1258, in
main
    compare_trees (args[0], args[1], name, options.threshold)
  File "/home/papus/lilypond/scripts/build/out/output-distance", line 969, in
compare_trees
    data.compare_trees (dir1, dir2)
  File "/home/papus/lilypond/scripts/build/out/output-distance", line 812, in
compare_trees
    (root, dirs, files) = os.walk (dir1).next ()
StopIteration
make: *** [local-check] Error 1

I'm on Ubuntu 10.04, 64 bit (vmware).

I shouldn't upload my cleaned warnings patch, if regressions don't run on a
virgin clone :p.
Any advice is welcome.

Thanks,
    Lőrinc



      

[c_sorensen, 2010-06-19 00:03:01]

On 6/18/10 5:04 PM, "Pap Lôrinc" <paplorinc@yahoo.com> wrote:

>>>  - I just did (make check?), it fails (I guess, I'm not sure what to look
>>> for).
>> This is absolutely critical for any large-scale code change.
>  - I have checked out a clean clone of lily and after running
> ./autogen.sh
> make all
> make check
> (and importing it into Eclipse which doesn't change anything, lily runs
> nicely) it gives me this error:
> [...]
> ake[2]: Leaving directory `/home/papus/lilypond/input/regression/abc2ly'
> rsync -L -a --exclude 'out-*' --exclude 'out' --exclude mf --exclude source
> --exclude mf /home/papus/lilypond/out/share ./out-test
> make[1]: Leaving directory `/home/papus/lilypond/input/regression/abc2ly'
> rm -rf /home/papus/lilypond/out/test-results
> mkdir -p /home/papus/lilypond/out/test-results
> /home/papus/lilypond/scripts/build/out/output-distance --create-images
> --output-dir /home/papus/lilypond/out/test-results
> input/regression/out-test-baseline input/regression/out-test/
> Traceback (most recent call last):
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 1261, in
> <module>
>     main()
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 1258, in
> main
>     compare_trees (args[0], args[1], name, options.threshold)
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 969, in
> compare_trees
>     data.compare_trees (dir1, dir2)
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 812, in
> compare_trees
>     (root, dirs, files) = os.walk (dir1).next ()
> StopIteration
> make: *** [local-check] Error 1

This looks like the ImageMagick error.

See Issue 908.

<http://code.google.com/p/lilypond/issues/detail?id=908>

HTH,

Carl

[paplorinc_yahoo.com, 2010-06-19 01:09:20]

> HTH
 - Thanks Carl, but I've applied the imagemagick patch, did a long clean and
make all, but the make check failed in the same place.


Version: ImageMagick 6.5.7-8 2009-11-26 Q16 http://www.imagemagick.org
Copyright: Copyright (C) 1999-2009 ImageMagick Studio LLC
Features: OpenMP

Is it too old? It's the latest ImageMagick for Ubuntu.
Any ideas?

Lőrinc




________________________________
From: Carl Sorensen <c_sorensen@byu.edu>
To: Pap Lôrinc <paplorinc@yahoo.com>; Graham Percival
<graham@percival-music.ca>; "carl.d.sorensen@gmail.com"
<carl.d.sorensen@gmail.com>; Han-Wen Nienhuys <hanwenn@gmail.com>;
"lilypond-devel@gnu.org" <lilypond-devel@gnu.org>;
"reply@codereview.appspotmail.com" <reply@codereview.appspotmail.com>
Sent: Sat, 19 June, 2010 3:02:52
Subject: Re: 100+ warnings and some cosmetics along the way (issue1724041)

On 6/18/10 5:04 PM, "Pap Lôrinc" <paplorinc@yahoo.com> wrote:

>>>  - I just did (make check?), it fails (I guess, I'm not sure what to look
>>> for).
>> This is absolutely critical for any large-scale code change.
>  - I have checked out a clean clone of lily and after running
> ./autogen.sh
> make all
> make check
> (and importing it into Eclipse which doesn't change anything, lily runs
> nicely) it gives me this error:
> [...]
> ake[2]: Leaving directory `/home/papus/lilypond/input/regression/abc2ly'
> rsync -L -a --exclude 'out-*' --exclude 'out' --exclude mf --exclude source
> --exclude mf /home/papus/lilypond/out/share ./out-test
> make[1]: Leaving directory `/home/papus/lilypond/input/regression/abc2ly'
> rm -rf /home/papus/lilypond/out/test-results
> mkdir -p /home/papus/lilypond/out/test-results
> /home/papus/lilypond/scripts/build/out/output-distance --create-images
> --output-dir /home/papus/lilypond/out/test-results
> input/regression/out-test-baseline input/regression/out-test/
> Traceback (most recent call last):
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 1261, in
> <module>
>     main()
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 1258, in
> main
>     compare_trees (args[0], args[1], name, options.threshold)
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 969, in
> compare_trees
>     data.compare_trees (dir1, dir2)
>   File "/home/papus/lilypond/scripts/build/out/output-distance", line 812, in
> compare_trees
>     (root, dirs, files) = os.walk (dir1).next ()
> StopIteration
> make: *** [local-check] Error 1

This looks like the ImageMagick error.

See Issue 908.

<http://code.google.com/p/lilypond/issues/detail?id=908>

HTH,

Carl


      

[Patrick McCarty, 2010-06-19 02:34:13]

On Fri, Jun 18, 2010 at 6:09 PM, Pap Lôrinc <paplorinc@yahoo.com> wrote:
>> HTH
>  - Thanks Carl, but I've applied the imagemagick patch, did a long clean and
> make all, but the make check failed in the same place.
>
> Version: ImageMagick 6.5.7-8 2009-11-26 Q16 http://www.imagemagick.org
> Copyright: Copyright (C) 1999-2009 ImageMagick Studio LLC
> Features: OpenMP
>
> Is it too old? It's the latest ImageMagick for Ubuntu.
> Any ideas?

Did you run

  make test-baseline

in the beginning?

Running `make test-baseline' is a prerequisite for checking the
regression test output.

See
http://lilypond.org/doc/v2.13/Documentation/contributor/identifying-code-regr...

-Patrick

[paPus, 2010-06-19 21:06:21]

Thank you Patrick, I have reinstalled imagemagick also, and now it was finally
working.
  It took me some time to get familiar with vmware, Linux (again), Eclipse CDT,
GIT, Lily etc :).
Thanks for the patience :).

  I've separated the warnings patch
(http://codereview.appspot.com/1724041/show). Since it's my first patch here, I
would kindly ask, that someone checks the regressions again.
I hope it's OK now :), please tell me, if it ain't :).

Thanks,
  Lőrinc