clang -Wimplicit-fallthrough and missing breaks
Stephan Bergmann
sbergman at redhat.com
Thu May 12 08:28:23 UTC 2016
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 at 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 at 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 at 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 at 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 at 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.)
More information about the LibreOffice
mailing list