Re: clang -Wimplicit-fallthrough and missing breaks

On 05/09/2016 11:34 AM, Stephan Bergmann wrote:
> On 05/09/2016 10:54 AM, Caolán McNamara wrote:
> > Does the clang version we use for all the spiffy warnings support
> > -Wimplicit-fallthrough ?
> >
> > I see firefox has a MOZ_FALLTHROUGH macro that expands
> > to [[clang::fallthrough]] for clang and just plain /*fall-through*/ for
> > everything else.
> >
> > If we had support for something like that, and could rewrite all our
> > current /*fall-through*/ and //FALL_THROUGH etc comments to that macro,
> > then perhaps we could enable this warning by default and we wouldn't
> > see as many of these missing breaks getting missed until the next
> > coverity run as I've been seeing over the last few weeks.
>
> Clang's -Wimplicit-fallthrough is quite old (at least available already
> back in Clang 3.2).  But not enabled with -Wall/-Wextra, so somehow
> always fell through the cracks of getting enabled for our code base.
>
> C++17 will have a standard [[fallthrough]] annotation, but for now we
> can #ifdef some SAL_FALLTHROUGH as [[clang::fallthrough]], for
> LIBO_INTERNAL_ONLY at least, in sal/types.h.  Will take a look.

So we now have SAL_FALLTHROUGH, see

> commit 14cd5182c5f64c43581c82db8c958369152226ac
> Author: Stephan Bergmann <sbergman@redhat.com>
> Date:   Tue May 10 16:42:16 2016 +0200
>
>     Replace fallthrough comments with new SAL_FALLTHROUGH macro
>
>     ...which (in LIBO_INTERNAL_ONLY) for Clang expands to [[clang::fallthrough]] in
>     preparation of enabling -Wimplicit-fallthrough.  (This is only relevant for
>     C++11, as neither C nor old C++ has a way to annotate intended fallthroughs.)
>
>     Could use BOOST_FALLTHROUGH instead of introducing our own SAL_FALLTHROUGH, but
>     that would require adding back in dependencies on boost_headers to many
>     libraries where we carefully removed any remaining Boost dependencies only
>     recently.  (At least make SAL_FALLTHROUGH strictly LIBO_INTERNAL_ONLY, so its
>     future evolution will not have any impact on the stable URE interface.)  C++17
>     will have a proper [[fallthroug]], eventually removing the need for a macro
>     altogether.
>
>     Change-Id: I342a7610a107db7d7a344ea9cbddfd9714d7e9ca

and for Clang builds -Wimplicit-fallthrough is enabled now.

* All the fallthrough comments (well, at least all the ones that my 
Clang -Wimplicit-fallthrough build actually warned about) are replaced 
with SAL_FALLTHROUGH.

* A number of places where there was no fallthrough comment, but which 
apparently intended to fall through, have been augmented with 
SAL_FALLTHROUGH.

* A handful of places where there was no fallthrough comment looked 
dubious to me:

> commit 01f787a21a9dd0116545fbaa13d0a073db5b5d74
> Author: Stephan Bergmann <sbergman@redhat.com>
> Date:   Wed May 11 17:09:13 2016 +0200
>
>     Mark dubious fallthrough cases as "SAL_FALLTHROUGH; //TODO ???"
>
>     Would be great if people knowing about the respective code areas could look into
>     these, and either change them into plain "SAL_FALLTHROUGH;" or "break;".
>
>     Change-Id: I6bd5e04bbb84452bea57d10946522b456c2ad5f0
>
>  sc/source/ui/drawfunc/fuins2.cxx   | 1 +
>  sc/source/ui/vba/vbacondition.cxx  | 1 +
>  sc/source/ui/vba/vbarange.cxx      | 1 +
>  sfx2/source/view/viewfrm.cxx       | 2 ++
>  sw/source/filter/html/css1atr.cxx  | 1 +
>  sw/source/filter/html/swhtml.cxx   | 1 +
>  sw/source/uibase/shells/basesh.cxx | 1 +
>  sw/source/uibase/utlui/content.cxx | 1 +
>  8 files changed, 9 insertions(+)

* Five cases have been identified as missing breaks, backports to -5-1 
pushed to gerrit.

* Three cases have been identified as presumably missing breaks.  If 
anybody is certain that they should be backported, please initiate that:

> commit 5ffd2c1595d1f67f5e4b14e48188a1f37f1956b5
> Author: Stephan Bergmann <sbergman@redhat.com>
> Date:   Tue May 10 17:52:35 2016 +0200
>
>     Presumably missing break in switch
>
>     Was like that at least since d2000efb31f864e912c6cf52760eea0e602b6893
>     "#i106421#: move msfilter to filter", but as clarified on IRC:
>
>     <vmiklos> sberg: doesn't look intended, i think ESCHER_Prop_lineDashing and
>     ESCHER_Prop_fNoLineDrawDash are supposed to be mutually exclusive.
>
>     Change-Id: I5ea92e6bdc9800c4511ca041c0572d1f9ffca49c
>
>  filter/source/msfilter/escherex.cxx | 2 ++
>  1 file changed, 2 insertions(+)

> commit bb86fd9e3c75464ac27fa1534e85a5ae236ec484
> Author: Stephan Bergmann <sbergman@redhat.com>
> Date:   Tue May 10 18:00:41 2016 +0200
>
>     Presumably missing break in switch
>
>     In fd069bee7e57ad529c3c0974559fd2d84ec3151a "initial import" the case
>     SDRDRAG_CROOK fell through to the default branch, but which was irrelevant, as
>     the default branch's if-branch would only hit if bCroner || bVertex, in which
>     case the SDRDRAG_CROOK's if-branch would already have hit and returned.  Then
>     dc1fddc142ab438775e2c1bae4a0e148d263ce0d "INTEGRATION: CWS
>     cropmaster2000_DEV300: #i83933# added interactive graphic cropping" moved the
>     case SDRDRAG_CROP in between.
>
>     Change-Id: I66939fc62416e0a442b02e674d90812ce76f3b2b
>
>  svx/source/svdraw/svdview.cxx | 1 +
>  1 file changed, 1 insertion(+)

> commit 498d3d9c4b1b1201f1a8eaeb52cfd0dd9eaa047b
> Author: Stephan Bergmann <sbergman@redhat.com>
> Date:   Tue May 10 18:11:27 2016 +0200
>
>     Presumably missing break in switch
>
>     The code was like this ever since fd069bee7e57ad529c3c0974559fd2d84ec3151a
>     "initial import", but it looks more like the break was always missing than that
>     it was an intended fallthrough.  Note how the symmetric FIRSTRIGHT case does end
>     in a break.  Also note that in the original code, the fallthrough case RIGHT
>     guarded its modifications with
>
>       if (bTbx || n <= nPos)
>           aInnerRect.Right() -= pCli->aSize.Width();
>       break;
>
>     (where the surrounding if got since removed), so it was presumably less likely
>     that an erroneous fallthrough actually caused any modifications.
>
>     Change-Id: Idf7ee117f1e22dee19343684a2f56fbf464bdb7f
>
>  sfx2/source/appl/workwin.cxx | 1 +
>  1 file changed, 1 insertion(+)

* Of course, there may be more lurking in (platform-specific, etc.) code 
that hasn't yet been built with Clang.  (I enabled 
-Wimplicit-fallthrough also for clang-cl on Windows, but haven't done a 
build with it yet.)