[vtk-developers] Warning in vtkExtractSelectedIds

[Bill Lorensen]

I think this shows the need for gerrit code review. When that loop was
added, it may have been caught by reviewers. It's not automatic but
does add more eyes to the code change.

Back in the day, at GE, we tried to  pair-review our contributions to
vtk and itk.

Bill

On Fri, Dec 30, 2011 at 4:30 PM, David Gobbi <david.gobbi at gmail.com> wrote:
> On Fri, Dec 30, 2011 at 2:04 PM, David Doria <daviddoria at gmail.com> wrote:
>> On Fri, Dec 30, 2011 at 3:44 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>>> The more that I look at it, the more convinced I am that the outer loop
>>> is not even supposed to be there.
>>>
>>>  860         for (vtkIdType j = 0; j < input->GetNumberOfPoints(); j++)
>>>
>>> http://vtk.org/gitweb?p=VTK.git;a=blob;f=Graphics/vtkExtractSelectedIds.cxx;hb=ecbbf7f7#l860
>>>
>>> Take a look at the commit where this line was added:
>>> http://vtk.org/gitweb?p=VTK.git;a=commitdiff;h=7030e24d#patch5
>>>
>>> You have to scroll down some to see the line, but when this line was
>>> first added it was missing curly braces and it really looks as if the
>>> whole line is just a big typo.
>>>
>>>  - David
>>
>> Once the patch is ready, we should probably also add a test:
>>
>> ~/src/VTK/Graphics/Testing/Cxx$ grep -ri "ExtractSelectedIds" .
>> ~/src/VTK/Graphics/Testing/Cxx$
>>
>> I just wrote this demo:
>> http://www.vtk.org/Wiki/VTK/Examples/Cxx/PolyData/ExtractSelectedIds
>>
>> It could be used as a starting point for a simple test.
>
> According to the dashboard, the class already has good coverage,
> because it is used by vtkExtractSelection.  Of course more tests
> are always good.
>
> This is one of those odd bugs that testing might never find.  The
> block of code that it was in had full test coverage. It seems that its
> only side-effect was to eat up a few extra CPU cycles, though it had
> the potential of causing an infinite loop.
>
> That's why, even with automated testing, it is always a good idea
> to have real people look over the code from time to time.
>
>  - David
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>
> Follow this link to subscribe/unsubscribe:
> http://www.vtk.org/mailman/listinfo/vtk-developers
>



-- 
Unpaid intern in BillsBasement at noware dot com