Re: Add "-Wimplicit-fallthrough" to default flags (was Re: pgsql: Support FETCH FIRST WITH TIES)

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Apr 11, 2020 at 02:47:34PM -0400, Tom Lane wrote:
>> Yeah, assorted buildfarm animals are mentioning that too.  I wonder
>> if we should add that to the default warning options selected by
>> configure?  I don't remember if that's been discussed before.

> I'm all for it.  It seems like a trap easy to catch up early, and we do want to
> enforce it anyway.  I'm attaching a simple patch for that if needed, hopefully
> with the correct autoconf version.

Poking around in the archives, it seems like the only previous formal
proposal to add -Wimplicit-fallthrough was in the context of a much
more aggressive proposal to make a lot of non-Wall warnings into
errors [1], which people did not like.

-Wimplicit-fallthrough does have some issues, eg it seems that it's
applied at a stage where gcc hasn't yet figured out that elog(ERROR)
doesn't return, so you need to add breaks after those.  But we had
sort of agreed that we could have it on-by-default in one relevant
discussion [2], and then failed to pull the trigger.

If we do this, I suggest we use -Wimplicit-fallthrough=4, which
uses a more-restrictive-than-default definition of how a "fallthrough"
comment can be spelled.  The default, per the gcc manual, is

           * -Wimplicit-fallthrough=3 case sensitively matches one of the
               following regular expressions:

               *<"-fallthrough">
               *<"@fallthrough@">
               *<"lint -fallthrough[ \t]*">
               *<"[ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?FALL(S |
               |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)?">
               *<"[ \t.!]*(Else,? |Intentional(ly)? )?Fall((s |
               |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)?">
               *<"[ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )?fall(s |
               |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?">

which to my eyes is not exactly encouraging a project-standard style
for these, plus it seems like it might accept some things we'd rather
it didn't.  The only more-restrictive alternative, short of disabling
the comments altogether, is

           * -Wimplicit-fallthrough=4 case sensitively matches one of the
               following regular expressions:

               *<"-fallthrough">
               *<"@fallthrough@">
               *<"lint -fallthrough[ \t]*">
               *<"[ \t]*FALLTHR(OUGH|U)[ \t]*">

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/B9FB1155-B39D-43C9-A7E6-B67E1C59E4CE%40gmail.com
[2] https://www.postgresql.org/message-id/flat/E1fDenm-0000C8-IJ%40gemulon.postgresql.org