Thread: Warnings in compile
Hi,
sitting here on my flight back I went through the list of all warnings gcc
spits out when using -Wextra. There are a whole lot of them (~ 1700) that
mostly (except one) fall into one of four classes:
- unused parameters: ~ 600
- some combination of signed and unsigned: ~ 600 Are we really sure that *all* compilers out there do handle this
correctly?
- missing initializer: ~ 500 Probably coming from us initialising structures only partially.
- empty body in else statement: 14 all in backend/commands/copy.c There are some #defines of the form #define foo
if(1){ ... } else that are called as foo; I see the need for the macro to expand as block, but what use hase the empty
else?
I assume that the only warning outside these classes is a false positive.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
Michael Meskes <meskes@postgresql.org> writes:
> - some combination of signed and unsigned: ~ 600
> Are we really sure that *all* compilers out there do handle this correctly?
The behavior is spelled out in the C spec, and always has been. You
might as well worry if they handle "if" correctly.
> There are some #defines of the form
> #define foo if(1) { ... } else
> that are called as foo;
> I see the need for the macro to expand as block, but what use hase the empty
> else?
That sounds both dangerous and against our coding conventions. The
standard way to do that is "do { ... } while (0)"
regards, tom lane
On Mon, May 25, 2009 at 10:19:40AM -0400, Tom Lane wrote:
> Michael Meskes <meskes@postgresql.org> writes:
> > - some combination of signed and unsigned: ~ 600
> > Are we really sure that *all* compilers out there do handle this correctly?
>
> The behavior is spelled out in the C spec, and always has been. You
> might as well worry if they handle "if" correctly.
Well this is probably because I got bitten by this once. Okay, granted it was
very long ago and the compiler was not state of the art.
> > There are some #defines of the form
> > #define foo if(1) { ... } else
> > that are called as foo;
>
> > I see the need for the macro to expand as block, but what use hase the empty
> > else?
>
> That sounds both dangerous and against our coding conventions. The
> standard way to do that is "do { ... } while (0)"
Which won't work here as the macros have continue and break commands in them.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
Michael Meskes <meskes@postgresql.org> writes:
> On Mon, May 25, 2009 at 10:19:40AM -0400, Tom Lane wrote:
>> That sounds both dangerous and against our coding conventions. The
>> standard way to do that is "do { ... } while (0)"
> Which won't work here as the macros have continue and break commands in them.
Oh, right, that was Bruce's "improvement" of the COPY code. I was less
than thrilled with it, but didn't have an easy alternative.
You can't just remove the "else", or it's unsafe; and I'm afraid that
changing the macros into "else {}" would still leave us with some
warnings about empty statements ...
regards, tom lane
On Mon, May 25, 2009 at 11:27:27AM -0400, Tom Lane wrote:
> Oh, right, that was Bruce's "improvement" of the COPY code. I was less
> than thrilled with it, but didn't have an easy alternative.
>
> You can't just remove the "else", or it's unsafe; and I'm afraid that
But why? What does this empty else accomplish? I'd like to understand the need
for it.
> changing the macros into "else {}" would still leave us with some
> warnings about empty statements ...
Hmm, apparently no, at least not on my gcc 4.3. As soon as I add the empty
braces, the warning is gone. But without really understanding the need for this
I certainly don't want to make that change. Maybe Bruce can comment.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
Michael Meskes <meskes@postgresql.org> writes:
> On Mon, May 25, 2009 at 11:27:27AM -0400, Tom Lane wrote:
>> You can't just remove the "else", or it's unsafe;
> But why? What does this empty else accomplish?
Consider
if (...) macro;else something-else;
Without the "else" in the macro, this code would be parsed
in a surprising fashion, ie else bound to the wrong if.
I'm afraid that "else {}" might not be any better --- it
might fail outright in this context.
[ thinks for a bit... ] What might be both safe and warning-free
is to code an explicit empty statement, viz macro body as
if (1) { ... } else ((void) 0)
regards, tom lane
On Mon, May 25, 2009 at 12:10:49PM -0400, Tom Lane wrote:
> Consider
>
> if (...)
> macro;
> else
> something-else;
> ...
Sure, but some/most/all macros are called as
MACRO;
No real reason there it seems.
> [ thinks for a bit... ] What might be both safe and warning-free
> is to code an explicit empty statement, viz macro body as
>
> if (1) { ... } else ((void) 0)
Will try, but probably not now. :-)
michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
Michael Meskes <meskes@postgresql.org> writes:
> On Mon, May 25, 2009 at 12:10:49PM -0400, Tom Lane wrote:
>> Consider
>>
>> if (...)
>> macro;
>> else
>> something-else;
> Sure, but some/most/all macros are called as
> MACRO;
> No real reason there it seems.
Well, they are called that way right now. The point of this discussion
is making the code safe against easily-foreseeable future changes.
Now, I'm privately of the opinion that those macros were a terrible idea
to begin with, because of the fact that they contain continue and break
statements; not only does that make them not act like self-contained
code, but they will break --- silently --- if anyone tries to put them
inside nested loops or switch statements. However, that doesn't seem
nearly as likely as trying to put them inside if-statements; so I'll
just grumble to myself while insisting that we at least keep them safe
against that case.
regards, tom lane
On Mon, May 25, 2009 at 12:10:49PM -0400, Tom Lane wrote:
> [ thinks for a bit... ] What might be both safe and warning-free
> is to code an explicit empty statement, viz macro body as
>
> if (1) { ... } else ((void) 0)
I just tried this and yes, it quietens gcc and probably is at least as save as
the old version. Therefore I just commit this small change.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
Tom Lane wrote:
> Michael Meskes <meskes@postgresql.org> writes:
> > On Mon, May 25, 2009 at 10:19:40AM -0400, Tom Lane wrote:
> >> That sounds both dangerous and against our coding conventions. The
> >> standard way to do that is "do { ... } while (0)"
>
> > Which won't work here as the macros have continue and break commands in them.
>
> Oh, right, that was Bruce's "improvement" of the COPY code. I was less
> than thrilled with it, but didn't have an easy alternative.
>
> You can't just remove the "else", or it's unsafe; and I'm afraid that
> changing the macros into "else {}" would still leave us with some
> warnings about empty statements ...
Wow, that must have been a long time ago because I had forgotten about
it (seems it was 2005-12-27). As least I added a macro comment:
/** These macros centralize code used to process line_buf and raw_buf buffers.* They are macros because they often do
continue/breakcontrol and to avoid* function call overhead in tight COPY loops.** We must use "if (1)" because "do {}
while(0)"overrides the continue/break* processing. See http://www.cit.gu.edu.au/~anthony/info/C/C.macros.*/
As I remember this was an attempt to implement Greenplum's optimizations
in a coherent manner.
I have added a comment about why "((void) 0)" is used.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +