[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Fabio Massimo Di Nitto]

The patch is pretty much self explanatory and depends on the other 2 patches I
already posted.

most of the subprojects already build fine with -Werror, but at least 2 won't.
Applying this patch will require a bit of effort from different maintainers
to fix build warnings. I also opened a bugzilla for it a while ago.

If there is agreement on this change, I can provide access and/or build logs
from CVS HEAD built on different architectures (other than usual x86* suspects).
I have handy ppc/sparc/parisc/ia64 at least. The build logs will be on Ubuntu
machines but I am sure you guys at RedHat can survive that :P

Thanks
Fabio

-- 
I'm going to make him an offer he can't refuse.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: noWerror.diff
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070826/5b59aa64/attachment.ksh>

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Fabio Massimo Di Nitto]

Fabio Massimo Di Nitto wrote:
> The patch is pretty much self explanatory and depends on the other 2 patches I
> already posted.
> 
> most of the subprojects already build fine with -Werror, but at least 2 won't.
> Applying this patch will require a bit of effort from different maintainers
> to fix build warnings. I also opened a bugzilla for it a while ago.
> 
> If there is agreement on this change, I can provide access and/or build logs
> from CVS HEAD built on different architectures (other than usual x86* suspects).
> I have handy ppc/sparc/parisc/ia64 at least. The build logs will be on Ubuntu
> machines but I am sure you guys at RedHat can survive that :P
> 
> Thanks
> Fabio
> 
> 

I have received no special comments so far and one ACK from David on this change.

I am going to hold off the patch until next Monday before applying because I'd
like to see if somebody has any objections and because I need to leave for an
urgent travel and won't be able to provide build logs as promised for a few days.

Cheers
Fabio

-- 
I'm going to make him an offer he can't refuse.

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Lon Hohberger]

On Sun, Aug 26, 2007 at 07:56:24AM +0200, Fabio Massimo Di Nitto wrote:
> The patch is pretty much self explanatory and depends on the other 2 patches I
> already posted.
> 
> most of the subprojects already build fine with -Werror, but at least 2 won't.
> Applying this patch will require a bit of effort from different maintainers
> to fix build warnings. I also opened a bugzilla for it a while ago.
> 
> If there is agreement on this change, I can provide access and/or build logs
> from CVS HEAD built on different architectures (other than usual x86* suspects).

I agree.  Clean code is a good thing.

However, others may / may not want to comment.

-- 
Lon Hohberger - Software Engineer - Red Hat, Inc.

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Ryan McCabe]

On Thu, Aug 30, 2007 at 01:38:46PM -0400, Lon Hohberger wrote:
> I agree.  Clean code is a good thing.
> 
> However, others may / may not want to comment.

GCC can still be really stupid and issue warnings about code that's
perfectly fine, for example:

[rmccabe at sublimit ~]$ cat test.c
int main(void) {
    int x, y = 1;
    if (y || x)
        x = 1;
    return 0;
}

[rmccabe at sublimit ~]$ gcc -O2 -Wall -Werror test.c -o test
cc1: warnings being treated as errors
test.c: In function ?main?:
test.c:3: warning: ?x? is used uninitialized in this function

Ryan

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Fabio M. Di Nitto]

Ryan McCabe wrote:

> 
> GCC can still be really stupid and issue warnings about code that's
> perfectly fine, for example:

Right.. but it is "usually" right.

> 
> [rmccabe at sublimit ~]$ cat test.c
> int main(void) {
>     int x, y = 1;
>     if (y || x)
>         x = 1;
>     return 0;
> }
> 
> [rmccabe at sublimit ~]$ gcc -O2 -Wall -Werror test.c -o test
> cc1: warnings being treated as errors
> test.c: In function ?main?:
> test.c:3: warning: ?x? is used uninitialized in this function
> 

This gcc catch looks good to me.

int x, y=1; <- doesn?t init x to 1. Only y.

and you are getting a warning on if(y || x).@this point only y is
initialized. So for what I can see it is valid.

and please flame hard if i am missing the point... i just come out of an
11 hours meeting ;)

Fabio

-- 
I'm going to make him an offer he can't refuse.

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Fabio M. Di Nitto]

Ryan McCabe wrote:
> On Thu, Aug 30, 2007 at 06:59:22PM +0100, Fabio M. Di Nitto wrote:
>> int x, y=1; <- doesn?t init x to 1. Only y.
>>
>> and you are getting a warning on if(y || x). at this point only y is
>> initialized. So for what I can see it is valid.
>>
>> and please flame hard if i am missing the point... i just come out of an
>> 11 hours meeting ;)
> 
> x is never evaluated because the || short-circuits once y evaluates to
> true, so x is never read before being initialized. This is kind of a
> contrived example, but I've seen GCC report these same kinds of bogus
> errors in code that isn't contrived.
> 
> Ryan

Oh ok.. of course you are right and I see the point.

Thanks
Fabio

-- 
I'm going to make him an offer he can't refuse.

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Fabio M. Di Nitto]

Lon Hohberger wrote:
> On Sun, Aug 26, 2007 at 07:56:24AM +0200, Fabio Massimo Di Nitto wrote:
>> The patch is pretty much self explanatory and depends on the other 2 patches I
>> already posted.
>>
>> most of the subprojects already build fine with -Werror, but at least 2 won't.
>> Applying this patch will require a bit of effort from different maintainers
>> to fix build warnings. I also opened a bugzilla for it a while ago.
>>
>> If there is agreement on this change, I can provide access and/or build logs
>> from CVS HEAD built on different architectures (other than usual x86* suspects).
> 
> I agree.  Clean code is a good thing.
> 
> However, others may / may not want to comment.
> 

We have nobody running behind us to switch so I will wait a few more
days before doing it and it?s not hardcoded anyway. a proper ./configure
--cflags can always override it.

Fabio

-- 
I'm going to make him an offer he can't refuse.

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Patrick Caulfield]

Patch to make qdisk compile ... I haven't tested this BTW

Patrick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: qdisk.patch
Type: text/x-patch
Size: 3171 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070831/41d63c1f/attachment.bin>

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Fabio M. Di Nitto]

Patrick Caulfield wrote:
> Patch to make qdisk compile ... I haven't tested this BTW
> 
> Patrick
> 

I received a couple of comments on IRC and I think it?s important to
make clear that I did not switch the project to -Werror.

Specifically cman/qdisk and rgmanager did embedded -Werror way before I
started changing the build system and the build flags have just been
respectfully kept across my changes.

The trigger here for this specific problem was triggered, as spotted by
David, the switch from -O0 to -O2.

Cheers
Fabio

-- 
I'm going to make him an offer he can't refuse.

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Lon Hohberger]

On Fri, Aug 31, 2007 at 01:19:49PM +0100, Fabio M. Di Nitto wrote:
> Patrick Caulfield wrote:
> > Patch to make qdisk compile ... I haven't tested this BTW
> > 
> > Patrick
> > 
> 
> I received a couple of comments on IRC and I think it?s important to
> make clear that I did not switch the project to -Werror.

My fault.  I got mixed up - for that, I apologize.  I know there was
talk about it at one point.


-- Lon


-- 
Lon Hohberger - Software Engineer - Red Hat, Inc.

[Cluster-devel] [RFC] Switch the entire project to use -Werror

[Lon Hohberger]

On Fri, Aug 31, 2007 at 01:02:38PM +0100, Patrick Caulfield wrote:
> Patch to make qdisk compile ... I haven't tested this BTW

Ack.

-- Lon

-- 
Lon Hohberger - Software Engineer - Red Hat, Inc.