pgsql: Clean up warnings from -Wimplicit-fallthrough.

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.)

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.

Discussion: https://postgr.es/m/15083.1525207729@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/41c912cad15955b5f9270ef3688a44e91d410d3d

Modified Files
--------------
contrib/btree_gin/btree_gin.c | 1 +
contrib/pageinspect/hashfuncs.c | 4 ++
src/backend/access/hash/hashfunc.c | 64 +++++++++++++++++++++++++------
src/backend/catalog/objectaddress.c | 1 +
src/backend/commands/explain.c | 3 +-
src/backend/commands/indexcmds.c | 1 +
src/backend/commands/trigger.c | 1 +
src/backend/executor/execMain.c | 1 +
src/backend/executor/execReplication.c | 2 +
src/backend/executor/nodeLockRows.c | 1 +
src/backend/executor/nodeModifyTable.c | 2 +
src/backend/parser/gram.y | 3 ++
src/backend/parser/parse_utilcmd.c | 2 +
src/backend/regex/regc_lex.c | 1 +
src/backend/regex/regcomp.c | 3 +-
src/backend/tcop/postgres.c | 9 +++--
src/backend/utils/adt/acl.c | 1 +
src/backend/utils/adt/datetime.c | 4 +-
src/backend/utils/adt/numeric.c | 1 +
src/backend/utils/adt/ruleutils.c | 2 +-
src/backend/utils/adt/timestamp.c | 19 ++++++++-
src/backend/utils/misc/guc.c | 4 +-
src/backend/utils/sort/tuplestore.c | 2 +-
src/bin/pgbench/pgbench.c | 2 +
src/interfaces/ecpg/pgtypeslib/interval.c | 2 +
src/interfaces/ecpg/preproc/ecpg.c | 4 +-
src/pl/plpgsql/src/pl_exec.c | 10 +++++
src/pl/tcl/pltcl.c | 3 +-
28 files changed, 128 insertions(+), 25 deletions(-)

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

On 2018-05-01 17:32:39 -0700, Andres Freund wrote:
> 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.

Pushed that way.

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2018-05-01 23:35:18 +0000, Tom Lane wrote:
>> Clean up warnings from -Wimplicit-fallthrough.

> I found one more oddity with the current committed state: ...
> It seems that gcc gets confused by the #ifdef ECONNRESET.

Yeah, there's a gcc bug or three about that:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77817

> ... 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.

I thought about that, and didn't like it much. I hoped maybe the gcc guys
would fix it soon, but after reading their bugzilla entry more closely,
it sounds like we shouldn't hold our breath. So if you want to do it
like that, OK by me.

regards, tom lane

On 5/1/18 23:33, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> On 2018-05-01 23:35:18 +0000, Tom Lane wrote:
>>> Clean up warnings from -Wimplicit-fallthrough.
>
>> I found one more oddity with the current committed state: ...
>> It seems that gcc gets confused by the #ifdef ECONNRESET.
>
> Yeah, there's a gcc bug or three about that:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77817
>
>> ... 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.
>
> I thought about that, and didn't like it much. I hoped maybe the gcc guys
> would fix it soon, but after reading their bugzilla entry more closely,
> it sounds like we shouldn't hold our breath. So if you want to do it
> like that, OK by me.

It sounds like we have a handle on all the potential issues for now, so
I think it would be OK to add this warning to the standard set on master.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services