Skip to content
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

ENH: Declare converting Point(v) constructors explicit #3237

Conversation

N-Dekker
Copy link
Contributor

Prevented implicit conversion of a value to Point. Aims to avoid
possible flaws, like the ones addressed by:

Pull request #3225
commit 5b71d63
"BUG: Quick-fix ContourSpatialObject::Update(), LINEAR_INTERPOLATION case"

Pull request #3228
commit 12a501c
"BUG: Fix SetCenterInObjectSpace calls in Registration test"

Pull request #3231
commit e4841aa
"STYLE: Clarify estimation ray position RayCastInterpolateImageFunction"

Deleted Point(nullptr_t), especially to avoid PointType p = 0.

@github-actions github-actions bot added area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation labels Feb 27, 2022
Copy link
Contributor

@Leengit Leengit left a comment

Choose a reason for hiding this comment

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

I like it, but I worry that there could be external code that depends upon the deprecated behavior. Do we need to use the usual mechanism for deprecation?

Prevented implicit conversion of a value to `Point`. Aims to avoid
possible flaws, like the ones addressed by:

Pull request InsightSoftwareConsortium#3225
commit 5b71d63
"BUG: Quick-fix ContourSpatialObject::Update(), LINEAR_INTERPOLATION case"

Pull request InsightSoftwareConsortium#3228
commit 12a501c
"BUG: Fix `SetCenterInObjectSpace` calls in Registration test"

Pull request InsightSoftwareConsortium#3231
commit e4841aa
"STYLE: Clarify estimation ray position `RayCastInterpolateImageFunction`"

Deleted `Point(nullptr_t)`, especially to avoid `PointType p = 0`.
Copy link
Contributor

@Leengit Leengit left a comment

Choose a reason for hiding this comment

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

Looks good

@N-Dekker
Copy link
Contributor Author

I like it, but I worry that there could be external code that depends upon the deprecated behavior. Do we need to use the usual mechanism for deprecation?

Thanks @Leengit I just added an ITK_LEGACY_REMOVE check, to allow legacy support. Please check the force-push! Note that it does not produce a "deprecate warning", it just allows users to get the old (legacy) behavior back, by ITK_LEGACY_REMOVE=OFF

@N-Dekker N-Dekker marked this pull request as ready for review February 28, 2022 17:29
@hjmjohnson hjmjohnson merged commit 8825834 into InsightSoftwareConsortium:master Feb 28, 2022
SimonRit pushed a commit to RTKConsortium/RTK that referenced this pull request Mar 1, 2022
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Mar 7, 2022
Until now, this constructor was only declared `explicit` for "future"
builds (when `ITK_LEGACY_FUTURE_REMOVE` would be enabled).

Follow-up to commit a890962
"ENH: Make Vector construction from scalar value explicit"
by Brian Helba, January 26, 2015

Added a _deleted_ `Vector(nullptr_t)` constructor, especially to avoid
erroneous user code like `VectorType v = 0`.

Analog to:
pull request InsightSoftwareConsortium#3237
commit 8825834
ENH: Declare converting `Point(v)` constructors `explicit`
hjmjohnson pushed a commit that referenced this pull request Mar 9, 2022
Until now, this constructor was only declared `explicit` for "future"
builds (when `ITK_LEGACY_FUTURE_REMOVE` would be enabled).

Follow-up to commit a890962
"ENH: Make Vector construction from scalar value explicit"
by Brian Helba, January 26, 2015

Added a _deleted_ `Vector(nullptr_t)` constructor, especially to avoid
erroneous user code like `VectorType v = 0`.

Analog to:
pull request #3237
commit 8825834
ENH: Declare converting `Point(v)` constructors `explicit`
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jul 25, 2022
Allowing ITK_LEGACY_REMOVE=0N for ITK version >= 5.3rc04, which made those constructors `explicit`:

 - ITK pull request InsightSoftwareConsortium/ITK#3237 commit InsightSoftwareConsortium/ITK@8825834 "ENH: Declare converting `Point(v)` constructors `explicit`"
 - ITK pull request InsightSoftwareConsortium/ITK#3255 commit InsightSoftwareConsortium/ITK@abb88cc "ENH: Make Vector constructor from scalar explicit, unless LEGACY enabled"
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jul 25, 2022
Allowing ITK_LEGACY_REMOVE=0N for ITK version >= 5.3rc04, which made those constructors `explicit`:

 - ITK pull request InsightSoftwareConsortium/ITK#3237 commit InsightSoftwareConsortium/ITK@8825834 "ENH: Declare converting `Point(v)` constructors `explicit`"
 - ITK pull request InsightSoftwareConsortium/ITK#3255 commit InsightSoftwareConsortium/ITK@abb88cc "ENH: Make Vector constructor from scalar explicit, unless LEGACY enabled"
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jul 25, 2022
Allowing ITK_LEGACY_REMOVE=0N for ITK version >= 5.3rc04, which made those constructors `explicit`:

 - ITK pull request InsightSoftwareConsortium/ITK#3237 commit InsightSoftwareConsortium/ITK@8825834 "ENH: Declare converting `Point(v)` constructors `explicit`"
 - ITK pull request InsightSoftwareConsortium/ITK#3255 commit InsightSoftwareConsortium/ITK@abb88cc "ENH: Make Vector constructor from scalar explicit, unless LEGACY enabled"
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 type:Enhancement Improvement of existing methods or implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants