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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes(at)postgresql(dot)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(at)postgresql(dot)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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes(at)postgresql(dot)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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes(at)postgresql(dot)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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes(at)postgresql(dot)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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Tom Lane wrote:
> Michael Meskes <meskes(at)postgresql(dot)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/break control 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +