[vtk-developers] coding standards & switch case label accidental fallthrough

Sean McBride sean at rogue-research.com
Wed Apr 8 18:25:52 EDT 2015


On Thu, 9 Apr 2015 07:59:27 +1000, Andrew Maclean said:

>I don't see the need for introducing yet another VTK_ macro for what is a
>rare and perfectly legal code construct that is picked up as a warning by
>one compiler type.
>
>You are just imposing another constraint on programmers for what is a
>perfectly legal construct.
>
>What warning level are you using? I don't think that I have seen this
>warning when building VTK.

clang has a neat warning flag called -Weverything, which enables literally every warning it has.  Of course many of those warnings we can't/shouldn't/won't fix, but it's a great way to discover warning flags you don't know about.  So on one of my bots I pass -Weverything then a slew of -Wno-<foo> warnings to disable the many we don't care about.

On occasion, I look at removing one of the -Wno-<foo> suppressions where it looks worthwhile.  Here I'm looking at removing the -Wno-implicit-fallthough suppression.

If you're curious about the exact flags I use, here they are:
<https://open.cdash.org/viewNotes.php?buildid=3761769>

>As David Lonie says it does have its uses.
>
>Even when looking at Common/DataModel/vtkHyperTreeGrid.cxx in your link
>below I think it is easier to read and understand without the added macro
>(line 1108 onwards). Similarly in a few of the other ones
>e.g. vtkImageMapToColors.cxx, vtkImageInterpolator.cxx that you have there.
>
>Having said this I am amazed at the number of missing break statements that
>you have found. It is a wonder some of this code ever worked! So the
>warning is good in that respect.

You've just answered your point in your first paragraph.  :)

I'm not proposing banning fall-through, just proposing an automated way to catch accidental instances of it.

If a programmer forgets a 'break', the dashboard can now catch it.  If she really meant to have no break, she can shut the compiler up with VTK_FALLTHROUGH.  As you probably saw in my merge request, some of the deliberate fall-throughs were at least annotated with a comment, but alas the compiler does not grok comments.

The current fall-through warnings are on my torture dashboard:
<https://open.cdash.org/viewBuildError.php?type=1&buildid=3761769>

I agree the VTK_FALLTHROUGH macro is a bit ugly, but the current codebase shows we can and do forget 'break' statements.  I'm not aware of any other automated way to catch this mistake...

FWIW, boost has an equivalent BOOST_FALLTHROUGH macro.

>On a more general vein I deeply appreciate the effort you have put into
>cleaning up VTK over the past year or so. The code base is so much better
>thanks to your efforts.

Nice of you to say, thanks!

Cheers,

-- 
____________________________________________________________
Sean McBride, B. Eng                 sean at rogue-research.com
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada




More information about the vtk-developers mailing list