Re: Use GCC's -Wlogical-op if possible
On Thu, Jun 18, 2009 at 12:32 PM, Gerald Pfeifer<gerald(a)pfeifer.com> wrote:
I verified this does not cause any extra warnings with GCC 4.4, whereas GCC 4.5 will become quite a bit more useful in that regard and thus help spot any issues.
As with -Wtype-limits that I suggested last year, I pledge to keep close an eye on this and to address any issues proactively as part of my nightly test builds.
Gerald
ChangeLog: Use GCC's -Wlogical-op if possible.
diff --git a/configure.ac b/configure.ac index bef311e..3f7a657 100644 --- a/configure.ac +++ b/configure.ac @@ -1385,8 +1385,9 @@ then WINE_TRY_CFLAGS([-fno-builtin],[AC_SUBST(BUILTINFLAG,"-fno-builtin")]) WINE_TRY_CFLAGS([-fno-strict-aliasing]) WINE_TRY_CFLAGS([-Wdeclaration-after-statement]) - WINE_TRY_CFLAGS([-Wwrite-strings]) + WINE_TRY_CFLAGS([-Wlogical-op]) WINE_TRY_CFLAGS([-Wtype-limits]) + WINE_TRY_CFLAGS([-Wwrite-strings])
dnl Check for noisy string.h saved_CFLAGS="$CFLAGS"
Causes 106 more warnings on 4.3.3 of this sort: tab.c:693: warning: logical ‘&&’ with non-zero constant will always evaluate as true cert.c:1627: warning: logical ‘||’ with non-zero constant will always evaluate as true -- -Austin
2009/6/19 Austin English <austinenglish(a)gmail.com>:
On Thu, Jun 18, 2009 at 12:32 PM, Gerald Pfeifer<gerald(a)pfeifer.com> wrote:
I verified this does not cause any extra warnings with GCC 4.4, whereas GCC 4.5 will become quite a bit more useful in that regard and thus help spot any issues.
As with -Wtype-limits that I suggested last year, I pledge to keep close an eye on this and to address any issues proactively as part of my nightly test builds.
Gerald
ChangeLog: Use GCC's -Wlogical-op if possible.
diff --git a/configure.ac b/configure.ac index bef311e..3f7a657 100644 --- a/configure.ac +++ b/configure.ac @@ -1385,8 +1385,9 @@ then WINE_TRY_CFLAGS([-fno-builtin],[AC_SUBST(BUILTINFLAG,"-fno-builtin")]) WINE_TRY_CFLAGS([-fno-strict-aliasing]) WINE_TRY_CFLAGS([-Wdeclaration-after-statement]) - WINE_TRY_CFLAGS([-Wwrite-strings]) + WINE_TRY_CFLAGS([-Wlogical-op]) WINE_TRY_CFLAGS([-Wtype-limits]) + WINE_TRY_CFLAGS([-Wwrite-strings])
Is there a reason why -Wwrite-strings is moved?
On Fri, 19 Jun 2009, Ben Klein wrote:
diff --git a/configure.ac b/configure.ac index bef311e..3f7a657 100644 --- a/configure.ac +++ b/configure.ac @@ -1385,8 +1385,9 @@ then WINE_TRY_CFLAGS([-fno-builtin],[AC_SUBST(BUILTINFLAG,"-fno-builtin")]) WINE_TRY_CFLAGS([-fno-strict-aliasing]) WINE_TRY_CFLAGS([-Wdeclaration-after-statement]) - WINE_TRY_CFLAGS([-Wwrite-strings]) + WINE_TRY_CFLAGS([-Wlogical-op]) WINE_TRY_CFLAGS([-Wtype-limits]) + WINE_TRY_CFLAGS([-Wwrite-strings]) Is there a reason why -Wwrite-strings is moved?
Alphabetical sorting, now the list gets longer. :-) Gerald
On Thu, 18 Jun 2009, Austin English wrote:
Causes 106 more warnings on 4.3.3 of this sort: tab.c:693: warning: logical ?&&? with non-zero constant will always evaluate as true cert.c:1627: warning: logical ?||? with non-zero constant will always evaluate as true
This is strange. In that case, let's hold off and I will work on addressing these first (testing with even more versions of GCC than I had done). Gerald
Gerald Pfeifer wrote:
On Thu, 18 Jun 2009, Austin English wrote:
Causes 106 more warnings on 4.3.3 of this sort: tab.c:693: warning: logical ?&&? with non-zero constant will always evaluate as true cert.c:1627: warning: logical ?||? with non-zero constant will always evaluate as true
This is strange. In that case, let's hold off and I will work on addressing these first (testing with even more versions of GCC than I had done).
Gerald
I have about 70 warnings with 4.3.2. I already sent two patches and found a few more bugs. If you want I can start sending in patches for those as well (don't want to duplicate work of course). -- Cheers, Paul.
On Fri, 19 Jun 2009, Paul Vriens wrote:
I have about 70 warnings with 4.3.2.
I already sent two patches and found a few more bugs. If you want I can start sending in patches for those as well (don't want to duplicate work of course).
Thanks for checking to help avoid duplicate work, Paul; I really appreciate that. Austin was kind enough to share his logs, and I now submitted patches for (more or less) everything I had seen on my side and could reproduce based on this input. If you want to go for the rest you are seeing after those few patches of mine, that would be great. If you could keep us updated on how it goes (ideally bringing warnings down to zero, of course ;-) that would be nice, too. Happy hacking! :) Gerald
On Fri, Jun 19, 2009 at 12:35 PM, Gerald Pfeifer<gerald(a)pfeifer.com> wrote:
On Fri, 19 Jun 2009, Paul Vriens wrote:
I have about 70 warnings with 4.3.2.
I already sent two patches and found a few more bugs. If you want I can start sending in patches for those as well (don't want to duplicate work of course).
Thanks for checking to help avoid duplicate work, Paul; I really appreciate that. Austin was kind enough to share his logs, and I now submitted patches for (more or less) everything I had seen on my side and could reproduce based on this input.
If you want to go for the rest you are seeing after those few patches of mine, that would be great. If you could keep us updated on how it goes (ideally bringing warnings down to zero, of course ;-) that would be nice, too.
Happy hacking! :)
Gerald
For those interested, here's the make log in Monday's git. 94 warnings for me. -- -Austin
Austin English wrote:
On Fri, Jun 19, 2009 at 12:35 PM, Gerald Pfeifer<gerald(a)pfeifer.com> wrote:
On Fri, 19 Jun 2009, Paul Vriens wrote:
I have about 70 warnings with 4.3.2.
I already sent two patches and found a few more bugs. If you want I can start sending in patches for those as well (don't want to duplicate work of course). Thanks for checking to help avoid duplicate work, Paul; I really appreciate that. Austin was kind enough to share his logs, and I now submitted patches for (more or less) everything I had seen on my side and could reproduce based on this input.
If you want to go for the rest you are seeing after those few patches of mine, that would be great. If you could keep us updated on how it goes (ideally bringing warnings down to zero, of course ;-) that would be nice, too.
Happy hacking! :)
Gerald
For those interested, here's the make log in Monday's git. 94 warnings for me.
Sent 3 patches based on your log. I have less (55) warnings on 4.3.2 btw, but most look like false positives. -- Cheers, Paul.
On Tue, 23 Jun 2009, Paul Vriens wrote:
For those interested, here's the make log in Monday's git. 94 warnings for me. Sent 3 patches based on your log.
I have less (55) warnings on 4.3.2 btw, but most look like false positives.
On my FreeBSD test system I am see no warnings triggered by -Wlogical-op any more. How does it look on your side? Depending on your findings, we may want to enable -Wlogical-op by default and I'd submit a patch to that end. Gerald
On Sun, Jan 3, 2010 at 4:13 AM, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
On Tue, 23 Jun 2009, Paul Vriens wrote:
For those interested, here's the make log in Monday's git. 94 warnings for me. Sent 3 patches based on your log.
I have less (55) warnings on 4.3.2 btw, but most look like false positives.
On my FreeBSD test system I am see no warnings triggered by -Wlogical-op any more. How does it look on your side?
ole32: usrmarshal.c:485: warning: logical ‘&&’ with non-zero constant will always evaluate as true usrmarshal.c:1098: warning: logical ‘&&’ with non-zero constant will always evaluate as true usrmarshal.c:1290: warning: logical ‘&&’ with non-zero constant will always evaluate as true oleaut32: variant.c:2090: warning: logical ‘||’ with non-zero constant will always evaluate as true variant.c:2090: warning: logical ‘&&’ with non-zero constant will always evaluate as true comctl32/tests: tab.c:520: warning: logical ‘&&’ with non-zero constant will always evaluate as true tab.c:540: warning: logical ‘&&’ with non-zero constant will always evaluate as true tab.c:563: warning: logical ‘&&’ with non-zero constant will always evaluate as true tab.c:978: warning: logical ‘&&’ with non-zero constant will always evaluate as true kernel32/tests: atom.c:70: warning: logical ‘||’ with non-zero constant will always evaluate as true -- -Austin
On Sun, 3 Jan 2010, Austin English wrote:
On my FreeBSD test system I am see no warnings triggered by -Wlogical-op any more. How does it look on your side? ole32: usrmarshal.c:485: warning: logical ?&&? with non-zero constant will always evaluate as true usrmarshal.c:1098: warning: logical ?&&? with non-zero constant will always evaluate as true usrmarshal.c:1290: warning: logical ?&&? with non-zero constant will always evaluate as true
oleaut32: variant.c:2090: warning: logical ?||? with non-zero constant will always evaluate as true variant.c:2090: warning: logical ?&&? with non-zero constant will always evaluate as true
comctl32/tests: tab.c:520: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:540: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:563: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:978: warning: logical ?&&? with non-zero constant will always evaluate as true
I had a patch for this one (comctl32/tests) which I received feedback on and need to brush up. I should be able to do so coming week. Anybody volunteering to look into the other ones?
kernel32/tests: atom.c:70: warning: logical ?||? with non-zero constant will always evaluate as true
Gerald
On 01/03/2010 10:12 PM, Gerald Pfeifer wrote:
On Sun, 3 Jan 2010, Austin English wrote:
On my FreeBSD test system I am see no warnings triggered by -Wlogical-op any more. How does it look on your side? ole32: usrmarshal.c:485: warning: logical ?&&? with non-zero constant will always evaluate as true usrmarshal.c:1098: warning: logical ?&&? with non-zero constant will always evaluate as true usrmarshal.c:1290: warning: logical ?&&? with non-zero constant will always evaluate as true
oleaut32: variant.c:2090: warning: logical ?||? with non-zero constant will always evaluate as true variant.c:2090: warning: logical ?&&? with non-zero constant will always evaluate as true
comctl32/tests: tab.c:520: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:540: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:563: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:978: warning: logical ?&&? with non-zero constant will always evaluate as true
I had a patch for this one (comctl32/tests) which I received feedback on and need to brush up. I should be able to do so coming week. Anybody volunteering to look into the other ones?
kernel32/tests: atom.c:70: warning: logical ?||? with non-zero constant will always evaluate as true
Gerald
The attached is what I have on my F12 box (gcc 4.4.2 20091222): -- Cheers, Paul.
2010/1/4 Paul Vriens <paul.vriens.wine(a)gmail.com>:
On 01/03/2010 10:12 PM, Gerald Pfeifer wrote:
On Sun, 3 Jan 2010, Austin English wrote:
On my FreeBSD test system I am see no warnings triggered by -Wlogical-op any more. How does it look on your side?
ole32: usrmarshal.c:485: warning: logical ?&&? with non-zero constant will always evaluate as true usrmarshal.c:1098: warning: logical ?&&? with non-zero constant will always evaluate as true usrmarshal.c:1290: warning: logical ?&&? with non-zero constant will always evaluate as true
oleaut32: variant.c:2090: warning: logical ?||? with non-zero constant will always evaluate as true variant.c:2090: warning: logical ?&&? with non-zero constant will always evaluate as true
comctl32/tests: tab.c:520: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:540: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:563: warning: logical ?&&? with non-zero constant will always evaluate as true tab.c:978: warning: logical ?&&? with non-zero constant will always evaluate as true
I had a patch for this one (comctl32/tests) which I received feedback on and need to brush up. I should be able to do so coming week. Anybody volunteering to look into the other ones?
kernel32/tests: atom.c:70: warning: logical ?||? with non-zero constant will always evaluate as true
Gerald
The attached is what I have on my F12 box (gcc 4.4.2 20091222):
-- Cheers,
Paul.
Most of them are false positives because of a problem between gcc 4.{3,4}.x and the strchr being defined as a macro in libc, see gcc bug #36513 [1]. My karmic box seems to output the same. [1] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513 -- Nicolas Le Cam
Gerald Pfeifer schrieb:
I had a patch for this one (comctl32/tests) which I received feedback on and need to brush up. I should be able to do so coming week. Anybody volunteering to look into the other ones?
kernel32/tests: atom.c:70: warning: logical ?||? with non-zero constant will always evaluate as true
Gerald
I dont see a reason to take that warning serious, as its not a problem in any case. in kernel32 it just happens because we use a Macro. Taking that case(v==1) out of the Macro leads to harder readable code. So IMHO i would not make -Wlogical-op the default. -- Best Regards, André Hentschel
2010/1/4 André Hentschel <nerv(a)dawncrow.de>:
I dont see a reason to take that warning serious, as its not a problem in any case. in kernel32 it just happens because we use a Macro. Taking that case(v==1) out of the Macro leads to harder readable code. So IMHO i would not make -Wlogical-op the default.
Not using that macro at all would be even more readable. I'm probably missing something obvious, but it seems to just do a wsprintfW() with "#%d" as format.
Henri Verbeet schrieb:
2010/1/4 André Hentschel <nerv(a)dawncrow.de>:
I dont see a reason to take that warning serious, as its not a problem in any case. in kernel32 it just happens because we use a Macro. Taking that case(v==1) out of the Macro leads to harder readable code. So IMHO i would not make -Wlogical-op the default.
Not using that macro at all would be even more readable. I'm probably missing something obvious, but it seems to just do a wsprintfW() with "#%d" as format. i can confirm that you are right.
-- Best Regards, André Hentschel
On Mon, 4 Jan 2010, Henri Verbeet wrote:
I dont see a reason to take that warning serious, as its not a problem in any case. in kernel32 it just happens because we use a Macro. Taking that case(v==1) out of the Macro leads to harder readable code. So IMHO i would not make -Wlogical-op the default. Not using that macro at all would be even more readable. I'm probably missing something obvious, but it seems to just do a wsprintfW() with "#%d" as format.
I see this has been comitted via b45d4aa161fbbb905fa9142d2757ff3f7952566d. Thanks! Looks like this warning is useful after all. ;-) In fact, doing a build with -Wlogical-op right now it turns out that with the change above and the work last year there aren't any open issues on that front any more and I'll submit a patch to make this used by default. Gerald
On Fri, 19 Jun 2009, Rein Klazes wrote:
cert.c:1627: warning: logical ?||? with non-zero constant will always evaluate as true That (in dlls/crypt32/tests) looks like a real bug to me.
Indeed. And Alexandre committed a fix of mine in the last 24 hours. ;-) Gerald
participants (8)
-
André Hentschel -
Austin English -
Ben Klein -
Gerald Pfeifer -
Henri Verbeet -
Nicolas Le Cam -
Paul Vriens -
Rein Klazes