Skip to content

WIP (Not ready): Add GradientImageFilter::OverrideBoundaryCondition(std::unique_ptr<BoundaryConditionType>) - #4533

Draft
N-Dekker wants to merge 5 commits into
InsightSoftwareConsortium:mainfrom
N-Dekker:GradientImageFilter-OverrideBoundaryCondition
Draft

WIP (Not ready): Add GradientImageFilter::OverrideBoundaryCondition(std::unique_ptr<BoundaryConditionType>)#4533
N-Dekker wants to merge 5 commits into
InsightSoftwareConsortium:mainfrom
N-Dekker:GradientImageFilter-OverrideBoundaryCondition

Conversation

@N-Dekker

Copy link
Copy Markdown
Contributor

Deprecated the old GradientImageFilter member function OverrideBoundaryCondition(BoundaryConditionType *) in favor of OverrideBoundaryCondition(std::unique_ptr<BoundaryConditionType>).

Aims to make it more clear that the filter takes the ownership of the specified BoundaryCondition object.

Triggered by a comment from Bradley Lowekamp (@blowekamp) at #4512 (comment)

@github-actions github-actions Bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module labels Mar 26, 2024
@N-Dekker
N-Dekker force-pushed the GradientImageFilter-OverrideBoundaryCondition branch from e6fd91e to ddf1f8d Compare March 26, 2024 09:57
@dzenanz
dzenanz requested a review from blowekamp March 26, 2024 11:51
@N-Dekker
N-Dekker force-pushed the GradientImageFilter-OverrideBoundaryCondition branch from ddf1f8d to 9843157 Compare April 1, 2024 20:24
N-Dekker added 3 commits April 2, 2024 11:36
Follow-up to pull request InsightSoftwareConsortium#4532
commit b4f0bb8
STYLE: Replace "the the" with "the" in comments
Added a helper typedef, for `GradientImageFilter::OverrideBoundaryCondition`
and its implementation.
Added `GradientImageFilter::OverrideBoundaryCondition` overload, to allows
specifying the argument by a `unique_ptr`. Aims to make it more clear that the
filter takes the ownership of the specified `BoundaryCondition` object.

Triggered by a comment from Bradley Lowekamp at
InsightSoftwareConsortium#4512 (comment)
on March 13, 2024.
@N-Dekker
N-Dekker force-pushed the GradientImageFilter-OverrideBoundaryCondition branch from 9843157 to 650d8ca Compare April 2, 2024 09:36
N-Dekker added 2 commits April 2, 2024 15:48
Aims to avoid errors from ITK.Linux.Python and ITK.macOS.Python like those from
https://open.cdash.org/viewBuildError.php?buildid=9511958:

> Wrapping/Modules/ITKImageGradient/itkGradientImageFilterPython.cpp:3856:70:
> error: call to implicitly-deleted copy constructor of
> `std::unique_ptr<itk::ImageBoundaryCondition<itk::Image<float, 2>>>`

Using ITK's current SWIG revision, 2024-03-26-master.
Deprecated (`ITK_FUTURE_LEGACY_REMOVE`) the overload of
`GradientImageFilter::OverrideBoundaryCondition` that has a raw pointer as
parameter, in favor of the overload that has a `unique_ptr` as parameter.
@N-Dekker
N-Dekker force-pushed the GradientImageFilter-OverrideBoundaryCondition branch from 650d8ca to 337e3ff Compare April 2, 2024 13:48
@N-Dekker

N-Dekker commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

For the record, I started a discussion on how unique_ptr parameters are supported by SWIG, at:

@N-Dekker N-Dekker changed the title Add GradientImageFilter::OverrideBoundaryCondition(std::unique_ptr<BoundaryConditionType>) WIP (Not ready): Add GradientImageFilter::OverrideBoundaryCondition(std::unique_ptr<BoundaryConditionType>) Apr 4, 2024
@hjmjohnson

Copy link
Copy Markdown
Member

@N-Dekker — I've opened #6714, which carries this idea forward. Thank you for it, and apologies it sat blocked this long: the diagnosis here was correct, and the design you proposed is the one that landed.

The SWIG blocker is gone. ITK now pins SWIG 4.3.0, which ships std_unique_ptr.i. I verified the whole chain rather than assuming it: castxml emits the method, pygccxml resolves the argument as ::std::unique_ptr<...> (not ?unknown?, so igenerator.py's skip_method no longer drops it), the %unique_ptr typemap binds, and the generated C++ compiles, links, and passes a runtime ownership test — the exact step that failed in 2024.

One trap worth recording for anyone doing this elsewhere: %unique_ptr must be given ITK's mangled alias (itkImageBoundaryConditionIF2), not the C++ spelling. The C++ spelling fails silently — SWIG falls back to the default typemap with no error or warning.

What #6714 changes relative to this PR
  • Same core idea, but the new method is named SetBoundaryCondition and the existing OverrideBoundaryCondition(T *) is deprecated under ITK_FUTURE_LEGACY_REMOVE rather than changed in place, so existing callers keep both compiling and their current ownership semantics.
  • Widened to the whole family. GradientImageFilter was the only one of six classes whose identically-signed OverrideBoundaryCondition took ownership; the other five now say explicitly in their docs that they do not. The rule: OverrideBoundaryCondition(T *) never takes ownership, and any owning API uses a different name and a unique_ptr parameter.
  • Adds GetBoundaryCondition / ResetBoundaryCondition, GTests that observe ownership via a destruction counter, a Python test for the wrapped behavior, and a migration-guide entry.

I'd suggest closing this PR in favor of #6714, but that's your call — I haven't automated it, and there's no Closes keyword in the new commit.

hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request Jul 27, 2026
OverrideBoundaryCondition(T *) adopts its argument in GradientImageFilter but
not in the five other classes that declare the same signature, so the name
alone cannot tell a caller who frees the object.

Add SetBoundaryCondition(std::unique_ptr<BoundaryConditionType>), whose
parameter type states the transfer, plus GetBoundaryCondition and
ResetBoundaryCondition.  Deprecate the owning OverrideBoundaryCondition under
ITK_FUTURE_LEGACY_REMOVE and document the remaining ones as non-owning.

Supersedes InsightSoftwareConsortium#4533
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Filtering Issues affecting the Filtering module type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants