Re: pgsql: Clean up warnings from -Wimplicit-fallthrough.

From: Andres Freund <andres(at)anarazel(dot)de>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: pgsql: Clean up warnings from -Wimplicit-fallthrough.
Date: 2018-05-02 00:32:39
Message-ID: 20180502003239.wfnqu7ekz7j7imm4@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

Hi,

On 2018-05-01 23:35:18 +0000, Tom Lane wrote:
> Clean up warnings from -Wimplicit-fallthrough.
>
> Recent gcc can warn about switch-case fall throughs that are not
> explicitly labeled as intentional. This seems like a good thing,
> so clean up the warnings exposed thereby by labeling all such
> cases with comments that gcc will recognize.
>
> In files that already had one or more suitable comments, I generally
> matched the existing style of those. Otherwise I went with
> /* FALLTHROUGH */, which is one of the spellings approved at the
> more-restrictive-than-default level -Wimplicit-fallthrough=4.
> (At the default level you can also spell it /* FALL ?THRU */,
> and it's not picky about case. What you can't do is include
> additional text in the same comment, so some existing comments
> containing versions of this aren't good enough.)

There's also a few cases in the LLVM using code, I'll clean that up momentarily.

> Testing with gcc 8.0.1 (Fedora 28's current version), I found that
> I also had to put explicit "break"s after elog(ERROR) or ereport(ERROR);
> apparently, for this purpose gcc doesn't recognize that those don't
> return. That seems like possibly a gcc bug, but it's fine because
> in most places we did that anyway; so this amounts to a visit from the
> style police.

I found one more oddity with the current committed state:
/home/andres/src/postgresql/src/interfaces/libpq/fe-secure.c: In function ‘pqsecure_raw_write’:
/home/andres/src/postgresql/src/interfaces/libpq/fe-secure.c:91:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
if (cond) \
^
/home/andres/src/postgresql/src/interfaces/libpq/fe-secure.c:362:5: note: in expansion of macro ‘REMEMBER_EPIPE’
REMEMBER_EPIPE(spinfo, true);
^~~~~~~~~~~~~~
/home/andres/src/postgresql/src/interfaces/libpq/fe-secure.c:366:4: note: here
case ECONNRESET:
^~~~

It seems that gcc gets confused by the #ifdef ECONNRESET. Somehow adding
a newline before cures it. But the nicer fix seems to be to move the
FALL THRU into the ifdef, which even seems more correct when nitpicking
in god mode.

Unless you've a better idea I'll fix that together with the LLVM stuff.

Greetings,

Andres Freund

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Andres Freund 2018-05-02 02:55:39 pgsql: Further -Wimplicit-fallthrough cleanup.
Previous Message Tom Lane 2018-05-01 23:38:35 pgsql: Fix some assorted compiler warnings on Windows.

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2018-05-02 00:51:48 Re: A few warnings on Windows
Previous Message Yuriy Zhuravlev 2018-05-02 00:20:51 Re: Is a modern build system acceptable for older platforms