<div dir="ltr">I'm going to try <span style="font-size:12.8000001907349px">-Weverything on my code to see what horrors it throws up.</span></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 9, 2015 at 8:25 AM, Sean McBride <span dir="ltr"><<a href="mailto:sean@rogue-research.com" target="_blank">sean@rogue-research.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, 9 Apr 2015 07:59:27 +1000, Andrew Maclean said:<br>
<br>
>I don't see the need for introducing yet another VTK_ macro for what is a<br>
>rare and perfectly legal code construct that is picked up as a warning by<br>
>one compiler type.<br>
><br>
>You are just imposing another constraint on programmers for what is a<br>
>perfectly legal construct.<br>
><br>
>What warning level are you using? I don't think that I have seen this<br>
>warning when building VTK.<br>
<br>
</span>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.<br>
<br>
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.<br>
<br>
If you're curious about the exact flags I use, here they are:<br>
<<a href="https://open.cdash.org/viewNotes.php?buildid=3761769" target="_blank">https://open.cdash.org/viewNotes.php?buildid=3761769</a>><br>
<span class=""><br>
>As David Lonie says it does have its uses.<br>
><br>
>Even when looking at Common/DataModel/vtkHyperTreeGrid.cxx in your link<br>
>below I think it is easier to read and understand without the added macro<br>
>(line 1108 onwards). Similarly in a few of the other ones<br>
>e.g. vtkImageMapToColors.cxx, vtkImageInterpolator.cxx that you have there.<br>
><br>
>Having said this I am amazed at the number of missing break statements that<br>
>you have found. It is a wonder some of this code ever worked! So the<br>
>warning is good in that respect.<br>
<br>
</span>You've just answered your point in your first paragraph.  :)<br>
<br>
I'm not proposing banning fall-through, just proposing an automated way to catch accidental instances of it.<br>
<br>
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.<br>
<br>
The current fall-through warnings are on my torture dashboard:<br>
<<a href="https://open.cdash.org/viewBuildError.php?type=1&buildid=3761769" target="_blank">https://open.cdash.org/viewBuildError.php?type=1&buildid=3761769</a>><br>
<br>
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...<br>
<br>
FWIW, boost has an equivalent BOOST_FALLTHROUGH macro.<br>
<span class=""><br>
>On a more general vein I deeply appreciate the effort you have put into<br>
>cleaning up VTK over the past year or so. The code base is so much better<br>
>thanks to your efforts.<br>
<br>
</span>Nice of you to say, thanks!<br>
<div class="HOEnZb"><div class="h5"><br>
Cheers,<br>
<br>
--<br>
____________________________________________________________<br>
Sean McBride, B. Eng                 <a href="mailto:sean@rogue-research.com">sean@rogue-research.com</a><br>
Rogue Research                        <a href="http://www.rogue-research.com" target="_blank">www.rogue-research.com</a><br>
Mac Software Developer              Montréal, Québec, Canada<br>
<br>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">___________________________________________<br>Andrew J. P. Maclean<br><br>___________________________________________</div>
</div>