Обсуждение: 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. +