Re: pgsql: Clean up warnings from -Wimplicit-fallthrough. - Mailing list pgsql-hackers

From Andres Freund
Subject Re: pgsql: Clean up warnings from -Wimplicit-fallthrough.
Date
Msg-id 20180502003239.wfnqu7ekz7j7imm4@alap3.anarazel.de
Whole thread Raw
Responses Re: pgsql: Clean up warnings from -Wimplicit-fallthrough.  (Andres Freund <andres@anarazel.de>)
Re: pgsql: Clean up warnings from -Wimplicit-fallthrough.  (Tom Lane <tgl@sss.pgh.pa.us>)
List 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


pgsql-hackers by date:

Previous
From: Yuriy Zhuravlev
Date:
Subject: Re: Is a modern build system acceptable for older platforms
Next
From: Thomas Munro
Date:
Subject: Re: A few warnings on Windows