New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix TransformPhysicalPointToIndex, TransformPhysicalPointToContinuousIndex [[nodiscard]]
warnings
#4000
Fix TransformPhysicalPointToIndex, TransformPhysicalPointToContinuousIndex [[nodiscard]]
warnings
#4000
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good.
Modules/Compatibility/Deprecated/include/itkVectorResampleImageFilter.hxx
Show resolved
Hide resolved
When the `bool` return value of a `TransformPhysicalPointToIndex(point, index)` call can be ignored, it is preferable (for performance reasons) to call the `TransformPhysicalPointToIndex(point)` overload that just returns the index, preventing `[[nodiscard]]` compile warnings. These cases are found in Visual Studio, using the following regular expression: ` [^ ]+->TransformPhysicalPointToIndex\(.*,`
Did find code like this: ContinuousIndexType index; image->TransformPhysicalPointToContinuousIndex(point, index); And replaced it by something like: const ContinuousIndexType index = image->template TransformPhysicalPointToContinuousIndex<ContinuousIndexValueType>(point); When the `bool` return value is ignored, it is preferable (for performance reasons) to call the overload that just returns the continuous index, preventing `[[nodiscard]]` compile warnings. These cases are found in Visual Studio, using the following regular expression: ` [^ ]+->TransformPhysicalPointToContinuousIndex\(.*,`
5f42e74
to
3f16044
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the warnings 💚
Possible follow-up noted inline.
@@ -38,8 +38,7 @@ ScalarChanAndVeseSparseLevelSetImageFilter<TInputImage, TFeatureImage, TOutputIm | |||
InputPointType origin = input->GetOrigin(); | |||
|
|||
// In the context of the global coordinates | |||
FeatureIndexType start; | |||
this->GetInput()->TransformPhysicalPointToIndex(origin, start); | |||
FeatureIndexType start = this->GetInput()->TransformPhysicalPointToIndex(origin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems many of these could be marked const
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks Matt! Maybe for another PR? (I would like to have this one merged as soon as it's "good enough", because it fixes a few warnings at https://open.cdash.org/index.php?project=Insight)
Fixed compile errors saying: > error C2039: 'ContinuousIndexType': is not a member of 'itk::ContinuousIndex' These compile errors were introduced by pull request InsightSoftwareConsortium#4000 commit 7cda5ba "COMP: Fix TransformPhysicalPointToContinuousIndex `nodiscard` warnings"
Fixed compile errors saying: > error C2065: 'cindex': undeclared identifier These compile errors were introduced by pull request InsightSoftwareConsortium#4000 commit 7cda5ba "COMP: Fix TransformPhysicalPointToContinuousIndex `nodiscard` warnings"
Added missing `template` keywords to TransformPhysicalPointToContinuousIndex calls in Nonunit/Review tests. Addressed compile errors from Ubuntu-22.04-gcc11.2-TBB-Debug saying: > error: expected primary-expression before '>' token These compile errors were introduced by pull request InsightSoftwareConsortium#4000 commit 7cda5ba "COMP: Fix TransformPhysicalPointToContinuousIndex `nodiscard` warnings"
Added missing `template` keywords to TransformPhysicalPointToContinuousIndex calls in Nonunit/Review tests. Addressed compile errors from Ubuntu-22.04-gcc11.2-TBB-Debug saying: > error: expected primary-expression before '>' token These compile errors were introduced by pull request InsightSoftwareConsortium#4000 commit 7cda5ba "COMP: Fix TransformPhysicalPointToContinuousIndex `nodiscard` warnings"
Added missing `template` keywords to TransformPhysicalPointToContinuousIndex calls in Nonunit/Review tests. Addressed compile errors from Ubuntu-22.04-gcc11.2-TBB-Debug saying: > error: expected primary-expression before '>' token These compile errors were introduced by pull request #4000 commit 7cda5ba "COMP: Fix TransformPhysicalPointToContinuousIndex `nodiscard` warnings"
Declared local "IndexType" variables in "Nonunit/Review" that are initialized by the result of a TransformPhysicalPointToIndex(point) call `const`. Follow-up to commit 49ece7f "COMP: Fix TransformPhysicalPointToIndex `nodiscard` warnings" Suggested by Matt McCormick at InsightSoftwareConsortium#4000 (comment)
Declared local "IndexType" variables in "Nonunit/Review" that are initialized by the result of a TransformPhysicalPointToIndex(point) call `const`. Follow-up to commit 49ece7f "COMP: Fix TransformPhysicalPointToIndex `nodiscard` warnings" Suggested by Matt McCormick at #4000 (comment)
Replacing
TransformPhysicalPointToIndex(point, index)
withindex = TransformPhysicalPointToIndex(point)
appeared to be a no-brainer, while replacing theTransformPhysicalPointToContinuousIndex
calls was somewhat more challenging... but still no rocket science.