Skip to content

BUG: fix a crash in QuadEdgeMeshFilter tests#2788

Merged
hjmjohnson merged 3 commits intoInsightSoftwareConsortium:masterfrom
dzenanz:quadEdgeMeshCrash
Oct 29, 2021
Merged

BUG: fix a crash in QuadEdgeMeshFilter tests#2788
hjmjohnson merged 3 commits intoInsightSoftwareConsortium:masterfrom
dzenanz:quadEdgeMeshCrash

Conversation

@dzenanz
Copy link
Copy Markdown
Member

@dzenanz dzenanz commented Oct 8, 2021

itkSquaredEdgeLengthDecimationQuadEdgeMeshFilterTest
itkQuadricDecimationQuadEdgeMeshFilterTest

were crashing in Debug mode on Visual Studio 2017.

The element is used for its type, so the actual access is optimized away in Release mode. In these tests, the cell/pixel data did not have an element with key 0, so trying to access it caused a crash.

@github-actions github-actions Bot added area:Core Issues affecting the Core module area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Oct 8, 2021
Comment thread Modules/IO/MeshBase/include/itkMeshFileWriter.hxx Outdated
@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Oct 8, 2021

tkSquaredEdgeLengthDecimationQuadEdgeMeshFilterTest
itkQuadricDecimationQuadEdgeMeshFilterTest
were crashing in Debug mode on Visual Studio 2017.

Nice catch! I guess this bug might cause a crash on any platform or compiler version, right?

@dzenanz dzenanz marked this pull request as draft October 8, 2021 18:43
@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Oct 8, 2021

Yes, it is a bug which can affect any platform and compiler.

I am switching it to draft until I have time to fix the compile errors caused by this change.

@dzenanz dzenanz marked this pull request as ready for review October 28, 2021 18:37
@dzenanz dzenanz marked this pull request as draft October 28, 2021 21:09
@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots and removed area:Core Issues affecting the Core module labels Oct 28, 2021
itkSquaredEdgeLengthDecimationQuadEdgeMeshFilterTest
itkQuadricDecimationQuadEdgeMeshFilterTest

were crashing in Debug mode on Visual Studio 2017.

The element is used for its type, so the actual access is optimized away in Release mode. In these tests, the cell/pixel data did not have an element with key 0, so trying to access it caused a crash.
@dzenanz dzenanz marked this pull request as ready for review October 28, 2021 22:26
@github-actions github-actions Bot added the area:Core Issues affecting the Core module label Oct 28, 2021
Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not tried, but 💯 for doing this Dženan.

Just to make sure, are commits 0b87d5e and 36d01ea required for the fix to work ? If so, would it be better to squash them into the same commit as the fix ?

@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Oct 29, 2021

The other two commits are not required. Just something I noticed while working on this bug.

@jhlegarreta
Copy link
Copy Markdown
Member

The other two commits are not required. Just something I noticed while working on this bug.

OK.

Copy link
Copy Markdown
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🥇

@hjmjohnson hjmjohnson merged commit 759b8ff into InsightSoftwareConsortium:master Oct 29, 2021
@dzenanz dzenanz deleted the quadEdgeMeshCrash branch October 29, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants