Обсуждение: Add -Wold-style-definition to CFLAGS?

Hi,

ISTM that it's our coding style that we use

something
my_paramless_func(void)
{
...
}

definitions rather than omitting the (void), which makes the function
look like an old-style function declaration. I somewhat regularly notice
such omissions during review, and fix them.

Since gcc has a warning detecting such definition, I think we ought to
automatically add it when available?

The attached patch makes configure do so, and also fixes a handful of
uses that crept in.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Since gcc has a warning detecting such definition, I think we ought to
> automatically add it when available?

+1

            regards, tom lane

Hi,

On 2020-05-09 14:15:01 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Since gcc has a warning detecting such definition, I think we ought to
> > automatically add it when available?
> 
> +1

Any opinion about waiting for branching or not?

- Andres

Andres Freund <andres@anarazel.de> writes:
> On 2020-05-09 14:15:01 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> Since gcc has a warning detecting such definition, I think we ought to
>>> automatically add it when available?

>> +1

> Any opinion about waiting for branching or not?

I'd be OK with pushing it now, but I dunno about other people.

If we do want to push this sort of thing now, the nearby changes
to enable fallthrough warnings should go in too.

            regards, tom lane

On Sat, May 09, 2020 at 07:11:56PM -0400, Tom Lane wrote:
> I'd be OK with pushing it now, but I dunno about other people.

Sounds like a good idea to me to apply this part now.

> If we do want to push this sort of thing now, the nearby changes
> to enable fallthrough warnings should go in too.

If we do that, merging this second part before beta1 is out looks like
a good compromise to me.
--
Michael

On 2020-May-09, Tom Lane wrote:

> If we do want to push this sort of thing now, the nearby changes
> to enable fallthrough warnings should go in too.

I'll get that sorted out tomorrow.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2020-05-09 19:11:56 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-05-09 14:15:01 -0400, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> Since gcc has a warning detecting such definition, I think we ought to
> >>> automatically add it when available?
> 
> >> +1
> 
> > Any opinion about waiting for branching or not?
> 
> I'd be OK with pushing it now, but I dunno about other people.

I did run into a small bit of trouble doing so. Those seem to make it a
mistake to target 13.

Unfortunately it turns out that our CFLAG configure tests don't reliably
work with -Wold-style-definition. The problem is that the generated
program contains 'int main() {...}', which obviously is an old-style
definition. Which then causes a warning, which in turn causes the cflag
tests to fail because we run them with ac_c_werror_flag=yes.

There's a pretty easy local fix, which is that we can just use
AC_LANG_SOURCE() instead of AC_LANG_PROGRAM()
PGAC_PROG_VARCC_VARFLAGS_OPT(). There seems to be little reason not to
do so.

But that still leaves us with a lot of unnecessary subsequent warnings
for other tests in config.log. They don't cause problems afaics, as
ac_c_werror_flag=yes isn't widely used, but it's still more noisy than
I'd like. And the likelihood of silent failures down the line seems
higher than I'd like.

Upstream autoconf has fixed this in 2014 (1717921a), but since they've
not bothered to release since then...

The easiest way that I can see to deal with that is to simply redefine
the relevant autoconf macro. For me that solves the vast majority of
these bleats in config.log.  That's not particularly pretty, but we have
precedent for it... Since it's just 16 lines, I think we can live with
that?

Comments?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Unfortunately it turns out that our CFLAG configure tests don't reliably
> work with -Wold-style-definition. The problem is that the generated
> program contains 'int main() {...}', which obviously is an old-style
> definition. Which then causes a warning, which in turn causes the cflag
> tests to fail because we run them with ac_c_werror_flag=yes.

Ugh.  I suspect main() might not be the only problem, either.

> Upstream autoconf has fixed this in 2014 (1717921a), but since they've
> not bothered to release since then...

I wonder if there's any way to light a fire under them.

> The easiest way that I can see to deal with that is to simply redefine
> the relevant autoconf macro. For me that solves the vast majority of
> these bleats in config.log.  That's not particularly pretty, but we have
> precedent for it... Since it's just 16 lines, I think we can live with
> that?

I don't really think that -Wold-style-definition is worth that.

            regards, tom lane

Hi,

On 2020-06-09 02:03:09 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Unfortunately it turns out that our CFLAG configure tests don't reliably
> > work with -Wold-style-definition. The problem is that the generated
> > program contains 'int main() {...}', which obviously is an old-style
> > definition. Which then causes a warning, which in turn causes the cflag
> > tests to fail because we run them with ac_c_werror_flag=yes.
> 
> Ugh.  I suspect main() might not be the only problem, either.

There's a few more, but they're far far less noisy. And I don't think
any change the results after the fix, because they don't check for
output on stderr (based on grepping through configure).


> > Upstream autoconf has fixed this in 2014 (1717921a), but since they've
> > not bothered to release since then...
> 
> I wonder if there's any way to light a fire under them.

There's been talk about working towards a release a few months back:
https://lists.gnu.org/archive/html/autoconf/2020-03/msg00003.html


> > The easiest way that I can see to deal with that is to simply redefine
> > the relevant autoconf macro. For me that solves the vast majority of
> > these bleats in config.log.  That's not particularly pretty, but we have
> > precedent for it... Since it's just 16 lines, I think we can live with
> > that?
> 
> I don't really think that -Wold-style-definition is worth that.

Well, we don't need it for that, strictly speaking. Just using
AC_LANG_SOURCE is enough...

Greetings,

Andres Freund