BUG: fix an infinite loop in itk::VoronoiDiagram2DGenerator #5400
Conversation
github-actions
bot
added
type:Bug
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for contributing a pull request! 🙏
Welcome to the ITK community! 🤗👋☀️
We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖
This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.
|
Don't know if you want a regression test, and if your framework can handle "infinite looping" type failures. |
|
A regression test is advantageous. I don't think we have an explicit support for infinite looping, but such a test would fail via timeout. |
github-actions
bot
added
type:Infrastructure
|
Added the original case by doing "monkey see monkey do", but it looks like I can't build locally due to IPFS + work firewall or something. |
ILoveGoulash
force-pushed
the
fix-voronoi-diagram-2d-generator-inf-loop
branch
2 times, most recently
from
f5ff22b to
62457de
Compare
dzenanz
left a comment
dzenanz
left a comment
There was a problem hiding this comment.
OK, I can reproduce locally an infinite loop with this test.
|
Nah, the full "http://..." link is probably what triggers it, GitHub prettifies it on display. |
ILoveGoulash
force-pushed
the
fix-voronoi-diagram-2d-generator-inf-loop
branch
from
62457de to
288b65e
Compare
Modules/Segmentation/Voronoi/include/itkVoronoiDiagram2DGenerator.hxx
Outdated
Show resolved
Hide resolved
|
@ILoveGoulash Thank you for your contribution. There are just a few final actions that need to be finalized so that we can incorporate this into the main tree. |
ILoveGoulash
force-pushed
the
fix-voronoi-diagram-2d-generator-inf-loop
branch
from
288b65e to
380c33d
Compare
|
Should be okay now. |
hjmjohnson
force-pushed
the
fix-voronoi-diagram-2d-generator-inf-loop
branch
2 times, most recently
from
8f868e4 to
c6ccef6
Compare
|
@ILoveGoulash I rebased and removed the merge conflict, then updated the commit message for the regression test to be more descriptive. |
|
Thanks! |
hjmjohnson
force-pushed
the
fix-voronoi-diagram-2d-generator-inf-loop
branch
from
c6ccef6 to
cec88e2
Compare
|
Ping. |
The loop exit condition was present in the code, but never wired and removed via InsightSoftwareConsortium@cd97879 Fixes InsightSoftwareConsortium#4386
Tests and resolves InsightSoftwareConsortium#4386 where specific values caused deadlocks.
hjmjohnson
force-pushed
the
fix-voronoi-diagram-2d-generator-inf-loop
branch
from
4aa2b26 to
ddccb03
Compare
| itkVoronoiDiagram2DInfiniteLoopTest(int argc, char * argv[]) | ||
| { | ||
| if (argc != 1) | ||
| { | ||
| std::cerr << "Takes no parameters, but " << argc << "parameters given" << std::endl; |
There was a problem hiding this comment.
When the test has no parameters, preferably make it a GoogleTest. GoogleTest unit tests are typically more "to the point", avoiding unnecessary verbosity.
| EdgeInfo back = curr; | ||
| while (!(rawEdges[i].empty())) | ||
| auto maxStop = rawEdges[i].size(); | ||
| while (!(rawEdges[i].empty()) && (maxStop != 0)) |
There was a problem hiding this comment.
Do I understand correctly that the check !(rawEdges[i].empty()) is redundant now? I mean, could it just say:
while ( maxStop > 0 )If so, I would prefer such a simplification.
There was a problem hiding this comment.
Couldn't simply be a for loop?
for ( auto maxStop = rawEdges[i].size(); maxStop != 0; --maxStop ) Maybe then rename maxStop to j:
for ( auto j = rawEdges[i].size(); j != 0; --j ) 4 participants
The loop exit condition was present in the code, but never wired and removed via cd97879
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.