Changes to allow compiling with -Werror using gcc 4.6.2 on i386.

Changes to allow compiling with -Werror using gcc 4.6.2 on i386.

[md5i, 2011-12-20 01:01:14]

A set of changes to allow lilypond to be built with -Werror using gcc 4.6.2 on
i386.  I don't know if these changes are desirable, but I am submitting them in
the hopes that they may be useful.

[Graham Percival, 2011-12-20 01:54:29]

wow, I'd love to enable -Werror!

You'll probably find a ton of more errors when running 'make check' --
apparently that compiles some stuff in flower/ that isn't compiled during a
normal make.

http://codereview.appspot.com/5489092/diff/1/lily/beaming-pattern.cc
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/5489092/diff/1/lily/beaming-pattern.cc#newcode187
lily/beaming-pattern.cc:187: I64 count = 1;  //default -- 1 base moments in a
beam
huh, I've never encountered I64 before... why does this particular int need to
be explicitly 64 bits?  If we really can't fit that data into 32 bits, it would
be nice to add a comment explaining why.

[md5i, 2011-12-20 02:12:12]

http://codereview.appspot.com/5489092/diff/1/lily/beaming-pattern.cc
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/5489092/diff/1/lily/beaming-pattern.cc#newcode187
lily/beaming-pattern.cc:187: I64 count = 1;  //default -- 1 base moments in a
beam
On 2011/12/20 01:54:29, Graham Percival wrote:
> huh, I've never encountered I64 before... why does this particular int need to
> be explicitly 64 bits?  If we really can't fit that data into 32 bits, it
would
> be nice to add a comment explaining why.
The variable count has to be I64, because it may be set to the result of
test_count, which is I64.  test_count is I64 because tuplet_count is I64, and
tuplet_count is I64 because factor.num() is I64.  In each case, to use a smaller
width could, in theory as far as the compiler is concerned, truncate a value. 
In practice, this almost certainly never happens.  The alternative is to
explicitly cast I64 values to int.  That may be a better approach, but I prefer
to take a conservative approach, and avoid casts whenever possible.

[md5i, 2011-12-20 02:52:34]

On 2011/12/20 01:54:29, Graham Percival wrote:

> You'll probably find a ton of more errors when running 'make check' --
> apparently that compiles some stuff in flower/ that isn't compiled during a
> normal make.

In point of fact, I found no additional errors when running make check.  Please
understand, though, that I have a 32-bit machine.  Other warnings may pup up
with a 64-bit compile.

[Carl, 2011-12-20 03:55:59]

This patch will collide with the outstanding patch fixing beaming, as the
variables have changed and are treated as int.

Personally, I see no reason to use I64 instead of int.  Neil Puttock changed the
numerator and denominators to be I64 in 2008, with commit
be65b81068e99ed855334f332c3176d8b4942a11

The patch message shows no reason for doing so, but he changed several files
from int to I64.

Neil, can you shed some light on this?  It seems to me unlikely that we need I64
precision in any of these variables.

[dak, 2011-12-20 08:28:58]

On 2011/12/20 03:55:59, Carl wrote:

> Personally, I see no reason to use I64 instead of int.  Neil Puttock changed
the
> numerator and denominators to be I64 in 2008, with commit
> be65b81068e99ed855334f332c3176d8b4942a11

I don't think that research on the history of such changes should be holding up
a general maintenance patch like this one.  The purpose of the maintenance patch
is to make matters consistent, not to revisit single design decisions.  I don't
think we ever kept Graham from applying an indentation patch because we did not
like some of the code he indented.

That does not mean that one should just keep quiet if one is surprised by some
of our code, but rather that one should file an independent issue.  The issue of
Neil's decision does not lie in md5i's responsibility.

[dak, 2011-12-20 08:32:27]

http://codereview.appspot.com/5489092/diff/1/lily/parser.yy
File lily/parser.yy (right):

http://codereview.appspot.com/5489092/diff/1/lily/parser.yy#newcode36
lily/parser.yy:36: /* Define to get rid of conversion warning, int -> int16_t. 
This has
I think the side effect of significantly increasing the parse table size here is
disproportionate.

We should look for other remedies.

[Reinhold, 2011-12-20 14:02:50]

Is anyone here using a 64-bit OS? There are ~150 compiler warnings when
compiling lilypond currently (see bug 1890), so -Werror can't be enabled on
64-bit systems.

[mail_philholmes.net, 2011-12-20 14:12:18]

----- Original Message ----- 
From: <reinhold.kainhofer@gmail.com>
To: <md5i.mail@gmail.com>; <graham@percival-music.ca>; 
<carl.d.sorensen@gmail.com>; <n.puttock@gmail.com>; <dak@gnu.org>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Tuesday, December 20, 2011 2:02 PM
Subject: Re: Changes to allow compiling with -Werror using gcc 4.6.2 on 
i386.(issue 5489092)


> Is anyone here using a 64-bit OS? There are ~150 compiler warnings when
> compiling lilypond currently (see bug 1890), so -Werror can't be enabled
> on 64-bit systems.
>
>
> http://codereview.appspot.com/5489092/


I use 64 bit on my build machine.  Waste of 8 gigs of memory otherwise.

--
Phil Holmes

[Graham Percival, 2011-12-20 16:27:08]

On Tue, Dec 20, 2011 at 02:52:34AM +0000, md5i.mail@gmail.com wrote:
> 
> In point of fact, I found no additional errors when running make check.
> Please understand, though, that I have a 32-bit machine.  Other warnings
> may pup up with a 64-bit compile.

hmm, was this with
  -Wextra -Wconversion -Werror
?
clang definitely finds more warnings, but I recall that gcc has a
bunch of additional warnings that aren't enable with -Wextra.  :(

Anyway, that's no reason to hold off on this patch (if David's
concerns about the parser can be resolved).  Simply being able to
compile with plain old
  -Werror
would still be a good step forward; we can slowly add -Wextra and
various other -W options over time.

Cheers,
- Graham

[md5i, 2011-12-21 02:56:37]

http://codereview.appspot.com/5489092/diff/1/lily/parser.yy
File lily/parser.yy (right):

http://codereview.appspot.com/5489092/diff/1/lily/parser.yy#newcode36
lily/parser.yy:36: /* Define to get rid of conversion warning, int -> int16_t. 
This has
On 2011/12/20 08:32:27, dak wrote:
> I think the side effect of significantly increasing the parse table size here
is
> disproportionate.
> 
> We should look for other remedies.

I'd love to see another solution, but this was the only good one I was able to
find.  The problem is the part of the resulting parser.cc that reads:

  yysetstate:
    *yyssp = yystate;

yyssp is a yytype_int16*, and yystate is int.  yytype_int16 can be changed via
the YYTYPE_INT16 macro, but the int cannot be.  Why is it an int?  I don't know.
 From how it is used in the code I see no reason it should not be a yytype_int16
as well.  Unfortunately, I could not figure out to make this happen, short of
postprocessing the resulting parser.cc file.

Otherwise, the only solution I can think of for this is to exempt parser.cc from
-Werror.

[md5i, 2011-12-21 02:58:12]

On 2011/12/20 14:02:50, Reinhold wrote:
> Is anyone here using a 64-bit OS? There are ~150 compiler warnings when
> compiling lilypond currently (see bug 1890), so -Werror can't be enabled on
> 64-bit systems.

Yes, this does not surprise me.  I wish I had a 64-bit system, but I'm not due
for an upgrade for another year or so.  My patch is a small maintenance patch,
only aimed at solving the problems I can see.

[md5i, 2011-12-21 03:01:42]

On 2011/12/20 16:27:08, Graham Percival wrote:
> On Tue, Dec 20, 2011 at 02:52:34AM +0000, mailto:md5i.mail@gmail.com wrote:
> > 
> > In point of fact, I found no additional errors when running make check.
> > Please understand, though, that I have a 32-bit machine.  Other warnings
> > may pup up with a 64-bit compile.
> 
> hmm, was this with
>   -Wextra -Wconversion -Werror
> ?

Nope.  Just with -Wconversion -Werror -Wall -W -Wno-pmf-conversions
-Woverloaded-virtual

If I get time, I'll try with -Wextra and see what I get

[dak, 2011-12-21 09:34:42]

On 2011/12/21 02:56:37, md5i wrote:
> http://codereview.appspot.com/5489092/diff/1/lily/parser.yy
> File lily/parser.yy (right):
> 
> http://codereview.appspot.com/5489092/diff/1/lily/parser.yy#newcode36
> lily/parser.yy:36: /* Define to get rid of conversion warning, int -> int16_t.


> I'd love to see another solution, but this was the only good one I was able to
> find.

Declaring a 16bit type to actually be 32bit with all the ensuing problems is not
what I consider a good solution.

>  The problem is the part of the resulting parser.cc that reads:
> 
>   yysetstate:
>     *yyssp = yystate;
> 
> yyssp is a yytype_int16*, and yystate is int.  yytype_int16 can be changed via
> the YYTYPE_INT16 macro, but the int cannot be.  Why is it an int?  I don't
know.
>  From how it is used in the code I see no reason it should not be a
yytype_int16
> as well.

I agree.

> Unfortunately, I could not figure out to make this happen, short of
> postprocessing the resulting parser.cc file.

That would be one option.
 
> Otherwise, the only solution I can think of for this is to exempt parser.cc
from
> -Werror.

Perhaps turn off this particular warning in the source via some #pragma or what
GCC has in stock for that purpose?

Or even do the postprocessing changing the definition of yystate.

And a bug report to Bison upstream as well.