Skip to content

ENH: Add GradientImageFilter::SetBoundaryCondition taking a unique_ptr - #6714

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-boundary-condition-ownership-api
Aug 21, 2026
Merged

ENH: Add GradientImageFilter::SetBoundaryCondition taking a unique_ptr#6714
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-boundary-condition-ownership-api

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Jul 26, 2026

Copy link
Copy Markdown
Member

GradientImageFilter::OverrideBoundaryCondition(T *) adopts its argument, but the five other classes declaring the same signature do not — so the name alone cannot tell a caller who frees the object. This adds SetBoundaryCondition(std::unique_ptr<BoundaryConditionType>), whose parameter type states the transfer, and deprecates the owning overload.

Stacked on #6707 — the first commit here is that PR's. Review only ENH: Add GradientImageFilter::SetBoundaryCondition taking a unique_ptr; this will rebase to a single commit once #6707 merges.

Supersedes #4533, which stalled in 2024 on SWIG being unable to wrap a unique_ptr parameter. That blocker is gone — see below.

Why #4533's SWIG blocker no longer applies

ITK now pins SWIG 4.3.0 (swig_version_min 4.3.0, Wrapping/Generators/SwigInterface/CMakeLists.txt:23), which ships std_unique_ptr.i. Verified end-to-end rather than assumed:

  • castxml emits the SetBoundaryCondition method nodes.
  • pygccxml resolves the argument as ::std::unique_ptr<...>, not ?unknown? — so igenerator.py's skip_method never fires and the method reaches the .i.
  • The %unique_ptr typemap binds: 8 SWIG_POINTER_RELEASE sites in the generated itkGradientImageFilterPython.cpp.
  • The generated C++ compiles, links, and runs. This is the exact step that failed in 2024.

One non-obvious trap: %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, and the only way to notice is to grep the generated C++ for SWIG_POINTER_RELEASE.

Scope: the whole OverrideBoundaryCondition family
Class Ownership Change
GradientImageFilter Takes New unique_ptr setter; old overload deprecated
ConstNeighborhoodIterator Does not Doc comment only
NeighborhoodOperatorImageFilter Does not Doc comment only
VectorNeighborhoodOperatorImageFilter Does not Doc comment only
MorphologyImageFilter Does not Doc comment only
ObjectMorphologyImageFilter Does not Doc comment only

The rule this establishes: OverrideBoundaryCondition(T *) never takes ownership; any owning API uses a different name and a std::unique_ptr parameter.

GPUNeighborhoodOperatorImageFilter::OverrideBoundaryCondition is entirely commented out, so it is left alone as unrelated cleanup.

Setter semantics: invalidation and the null guard

SetBoundaryCondition and ResetBoundaryCondition call Modified(). Without it, ProcessObject::Update() short-circuits on MTime and a boundary-condition change after an initial Update() would silently return gradients computed with the previous condition. Modified() was added to the deprecated OverrideBoundaryCondition as well, which has the same defect on main today.

SetBoundaryCondition throws when handed an empty unique_ptr. The boundary condition is dereferenced unconditionally — itkNeighborhoodAccessorFunctor.h:88 does boundaryCondition->operator()(...) with no null check, reached from itkConstNeighborhoodIterator.h:208 on any out-of-bounds sample — so the filter must always hold a usable one.

Both were raised by Greptile on the first push and are covered by SetBoundaryConditionModifiesFilter and SetBoundaryConditionRejectsNull.

Deprecation blast radius

Audited before deprecating: exactly one in-tree owning call site (itkGradientImageFilterTest.cxx, migrated here) and zero downstream GitHub hits for OverrideBoundaryCondition GradientImageFilter.

The deprecated overload is kept under #if !defined(ITK_FUTURE_LEGACY_REMOVE) with a \deprecated tag, so existing new-passing callers keep compiling and keep their ownership semantics.

Test plan (all run locally)
Check Result
itkGradientImageFilterOwnershipPythonTest Passed
pre-commit run --all-files 29 hooks passed, no files modified
GTests, ITK_LEGACY_REMOVE=OFF 8 pass
GTests, ITK_FUTURE_LEGACY_REMOVE=ON + ITK_LEGACY_REMOVE=ON Compiles clean; deprecated test correctly excluded; 7 pass

Six new GTests. Four use a destruction-counting boundary condition to observe ownership directly: the unique_ptr setter adopts, Get/Reset behave, the non-owning family does not adopt, and the deprecated overload still adopts. Two more cover pipeline invalidation and the null guard.

The Python test asserts thisown transfers on SetBoundaryCondition, that re-offering an already-adopted object raises RuntimeError rather than double-freeing, and that the deprecated overload keeps its DISOWN behavior.

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Documentation Issues affecting the Documentation module labels Jul 26, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review July 27, 2026 00:02
@greptile-apps

This comment was marked as resolved.

Comment thread Modules/Filtering/ImageGradient/include/itkGradientImageFilter.hxx
Comment thread Modules/Filtering/ImageGradient/include/itkGradientImageFilter.hxx
@hjmjohnson
hjmjohnson force-pushed the enh-boundary-condition-ownership-api branch from 7afe436 to bafe84e Compare July 27, 2026 00:15
@hjmjohnson
hjmjohnson requested a review from N-Dekker July 27, 2026 12:34
@hjmjohnson
hjmjohnson requested a review from dzenanz August 21, 2026 14:32

@dzenanz dzenanz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did not take a look at the tests. The rest looks good.

Comment thread Modules/Filtering/ImageGradient/test/itkGradientImageFilterTest.cxx Outdated
@hjmjohnson
hjmjohnson force-pushed the enh-boundary-condition-ownership-api branch from bafe84e to 0251a53 Compare August 21, 2026 19:48
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
@github-actions github-actions Bot removed the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Aug 21, 2026
@hjmjohnson
hjmjohnson force-pushed the enh-boundary-condition-ownership-api branch from 0251a53 to 6203371 Compare August 21, 2026 19:54
@hjmjohnson

Copy link
Copy Markdown
Member Author

Two force-pushes: first is a plain rebase on main (92 commits), second is the comment removal only.

Local validation on the rebased tip, -DModule_ITKImageGradient=ON plus the IO modules the fixtures need: build ok, ctest -R Gradient 100% tests passed out of 33, including all eight GradientImageFilter.* GTests. pre-commit run --all-files clean.

@dzenanz dzenanz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, I didn't look at the new tests.

@hjmjohnson
hjmjohnson merged commit 07ae667 into InsightSoftwareConsortium:main Aug 21, 2026
14 of 16 checks passed
@hjmjohnson
hjmjohnson deleted the enh-boundary-condition-ownership-api branch August 22, 2026 15:15
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:Documentation Issues affecting the Documentation module area:Filtering Issues affecting the Filtering module area:Python wrapping Python bindings for a class type:Enhancement Improvement of existing methods or implementation 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