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

[Andrew Maclean]

I'm going to try -Weverything on my code to see what horrors it throws up.

On Thu, Apr 9, 2015 at 8:25 AM, Sean McBride <sean at rogue-research.com>
wrote:

> 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
>
>
>


-- 
___________________________________________
Andrew J. P. Maclean

___________________________________________
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20150409/d7594a06/attachment.html>