Warnings in compile

Started by Michael Meskesover 16 years ago10 messages

meskes@postgresql.org

over 16 years ago

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!

Tom Lane

tgl@sss.pgh.pa.us

over 16 years ago

In reply to: Michael Meskes (#1)

Re: Warnings in compile

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

Michael Meskes

meskes@postgresql.org

over 16 years ago

In reply to: Tom Lane (#2)

Re: Warnings in compile

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.

Tom Lane

tgl@sss.pgh.pa.us

over 16 years ago

In reply to: Michael Meskes (#3)

Re: Warnings in compile

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

Michael Meskes

meskes@postgresql.org

over 16 years ago

In reply to: Tom Lane (#4)

Re: Warnings in compile

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.

Tom Lane

tgl@sss.pgh.pa.us

over 16 years ago

In reply to: Michael Meskes (#5)

Re: Warnings in compile

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

Michael Meskes

meskes@postgresql.org

over 16 years ago

In reply to: Tom Lane (#6)

Re: Warnings in compile

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!

Tom Lane

tgl@sss.pgh.pa.us

over 16 years ago

In reply to: Michael Meskes (#7)

Re: Warnings in compile

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

Michael Meskes

meskes@postgresql.org

over 16 years ago

In reply to: Tom Lane (#6)

Re: Warnings in compile

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.

#10

Bruce Momjian

bruce@momjian.us

over 16 years ago

In reply to: Tom Lane (#4)

Re: Warnings in compile

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/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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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