Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#6349 closed enhancement (fixed)

[patch] GCC2/GCC4 warnings and -Werror

Reported by: kaliber Owned by: axeld
Priority: normal Milestone: R1
Component: Build System Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

I've fixed over 100 warnings. Changes are gcc2 and gcc4 compatible.

Attachments (2)

gcc-warnings.patch (58.7 KB) - added by kaliber 9 years ago.
gcc-warnings-fix.patch (11.3 KB) - added by kaliber 9 years ago.
Fixes after code review on mailing list

Download all attachments as: .zip

Change History (16)

Changed 9 years ago by kaliber

Attachment: gcc-warnings.patch added

comment:1 Changed 9 years ago by kaliber

Has a Patch: set

comment:2 Changed 9 years ago by zooey

Thanks for squashing all those warnings, kaliber!

I think the patch looks fine upon first scan. The only thing I'm unsure about is all the added parentheses in stuff like

if (a && b || c)

Maybe we should add -Wno-parentheses to our gcc4 builds?

comment:3 in reply to:  2 Changed 9 years ago by zooey

Owner: changed from bonefish to zooey
Status: newin-progress

Replying to zooey:

Maybe we should add -Wno-parentheses to our gcc4 builds?

I no longer think we should, since -Wparentheses indicates problems with missing parens around & (bitwise and) too, and we've already seen a lot of these bugs.

comment:4 Changed 9 years ago by zooey

Resolution: fixed
Status: in-progressclosed

Applied patch in hrev37670.

Changed 9 years ago by kaliber

Attachment: gcc-warnings-fix.patch added

Fixes after code review on mailing list

comment:5 Changed 9 years ago by kaliber

Resolution: fixed
Status: closedreopened

I've attached fixes after code review on mailing list.

comment:6 Changed 9 years ago by zooey

Owner: changed from zooey to axeld
Status: reopenedassigned

comment:7 Changed 9 years ago by kaliber

Axel has promised to apply the patch.

comment:8 Changed 9 years ago by stippi

Reviewed the second patch and it looks fine. I looked closer at the Firewire patch and I am wondering why you moved the logging function in the common accelerant code to be static, do you remember?

comment:9 Changed 9 years ago by stippi

The first patch contains many unwanted changes which you fixed in the second patch, but it still seems to contain some changes that don't appear in the second patch. Is this intentional?

comment:10 Changed 9 years ago by stippi

Applied the second patch in hrev39320 sans the common accelerant logging function change. Leaving ticket open until it is confirmed fixed. Thanks for your work and sorry for the delay!

Last edited 9 years ago by stippi (previous) (diff)

comment:11 in reply to:  8 Changed 9 years ago by kaliber

Replying to stippi:

Reviewed the second patch and it looks fine. I looked closer at the Firewire patch and I am wondering why you moved the logging function in the common accelerant code to be static, do you remember?

Here is explanation. Search for "This one doesn't belong here".

comment:12 Changed 9 years ago by stippi

Resolution: fixed
Status: assignedclosed

Oh thanks! Sorry, somehow I completely missed that the first patch was actually already applied. Should have read all the comments...

comment:13 Changed 9 years ago by stippi

Forgot to mention: Applied the remaining two hunks in hrev39321.

comment:14 Changed 9 years ago by axeld

Thanks! And sorry kaliber, a reminder is always welcome, though :-)

Note: See TracTickets for help on using tickets.