COMP: Adopt VXL private vnl_vector ivars via protected_set_data

[hjmjohnson]

Switch itk::Array<T> to use vnl_vector::protected_set_data(...) and turn off VXL_USE_HISTORICAL_PROTECTED_IVARS so ITK builds against modern VXL trees, including VXL 7.0.0 where vnl_vector::data and num_elmts are private.

Why protected_set_data

VXL added vnl_vector::protected_set_data(T*, size_t, bool) specifically as the supported transition path for derived classes (such as itk::Array<T>) that previously assigned data and num_elmts directly. The header comments it as:

This function is a work around for transitioning to data members being private

itk::Array<T> is the only ITK class that reaches into those base members directly, so the source-side change is localized to Modules/Core/Common/include/itkArray.hxx. Each direct assignment is rewritten as a protected_set_data call that preserves the existing semantics (the third argument tracks vnl_vector's own m_LetArrayManageMemory flag; ownership is still managed by Array<T>::m_LetArrayManageMemory).

Provenance: when did protected_set_data first ship?

vnl_vector::protected_set_data was added in upstream VXL commit ccdcf37b27 ("ENH: Add options to manage backward compatibility") on 2019-10-06.

The commit landed on master long before any tag in the present tag chain pointed at it, so the first released VXL version that carries it is v3.3.0 (project VERSION 3.3.0.0, released 2021-05-24).

Tag	project VERSION	Released	Has protected_set_data?
v3.3.0	3.3.0.0	2021-05-24	✅ first appearance

The Modules/ThirdParty/VNL/CMakeLists.txt minimum for ITK_USE_SYSTEM_VXL is bumped from 3.0.0 to 4.0.0 to ensure any external VXL is recent enough to contain the helper.

CMake configuration changes

• find_package(VXL 3.0.0 REQUIRED) → find_package(VXL 4.0.0 REQUIRED)
• VXL_USE_HISTORICAL_IMPLICIT_CONVERSIONS is now unconditionally OFF (previously gated on ITK_LEGACY_REMOVE OR ITK_FUTURE_LEGACY_REMOVE).
• VXL_USE_HISTORICAL_PROTECTED_IVARS flips from ON to OFF, which is what makes vnl_vector::data / num_elmts private and is the trigger for the source-side change above.

Test plan

• pre-commit run --all-files passes against the rewritten tip.
• Full ITK build (ninja -C build-vxl700 -j10) succeeds against a VXL 7.0.0 tree configured with VXL_USE_HISTORICAL_PROTECTED_IVARS=OFF.
• CI: GitHub Actions and Azure DevOps pipelines.

[greptile-apps]

Greptile Summary

This PR migrates itk::Array<T> away from direct access to vnl_vector's protected members (data, num_elmts) to the new protected_set_data transition helper, and sets VXL_USE_HISTORICAL_PROTECTED_IVARS=OFF so ITK can build against VXL 7.0.0. The CMake changes are correct, but all five protected_set_data call sites pass a hardcoded false for the ownership flag, which breaks the memory-management contract in several ways.

• Constructor, SetData, SetDataSameSize: when LetArrayManageMemory=true, vnl's ownership flag is false, so neither the Array destructor nor vnl's destructor frees the buffer → memory leak.
• SetSize: after protected_set_data(nullptr, 0, false), vnl's m_LetArrayManageMemory is false; the subsequent set_size call reaches vnl_vector_alloc_blah which asserts that flag is true → assertion failure in debug builds and a silent allocation leak in release builds.
• CMakeLists.txt: two mark_as_advanced trailing comments still say these flags "must track ITK LegacyRemove setting", which is no longer true.

Confidence Score: 3/5

Not safe to merge as-is: all five protected_set_data call sites pass a hardcoded false for the ownership flag, which silently leaks memory for the LetArrayManageMemory=true path and causes an assertion failure (debug) or allocation leak (release) inside SetSize for any Array wrapping external memory.

The core memory-management invariant that made the old direct-member approach work — vnl frees data when non-null — is broken by always telling vnl it does not own the buffer. Three data-introduction sites (constructor, SetData, SetDataSameSize) never free caller-owned buffers, and SetSize hits a vnl assertion every time it is called on a non-owning Array.

itkArray.hxx needs all five protected_set_data call sites reviewed; the third argument must reflect actual ownership intent, not a fixed false.

Important Files Changed

Filename	Overview
Modules/Core/Common/include/itkArray.hxx	Rewrites 5 direct vnl_vector::data/num_elmts assignments to protected_set_data calls, but always passes false as the ownership flag — causing a memory leak when LetArrayManageMemory=true (constructor, SetData, SetDataSameSize) and an assertion failure / silent memory leak in SetSize because set_size asserts m_LetArrayManageMemory before allocating.
Modules/ThirdParty/VNL/CMakeLists.txt	Bumps minimum system-VXL version from 3.0.0 to 4.0.0, makes VXL_USE_HISTORICAL_IMPLICIT_CONVERSIONS unconditionally OFF (removing the legacy gate), and flips VXL_USE_HISTORICAL_PROTECTED_IVARS from ON to OFF; two stale inline comments in mark_as_advanced still say these track the legacy-remove setting.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Array as itkArray
    participant VNL as vnl_vector

    Note over Array,VNL: Array(datain, sz, true) - ownership case
    Caller->>Array: "Array(datain, sz, true)"
    Array->>VNL: "protected_set_data(datain, sz, false)"
    Note right of VNL: "m_LetArrayManageMemory = false (WRONG)"
    Note over Array,VNL: At destruction
    Array->>Array: "!m_LetArrayManageMemory is false, skip null-out"
    VNL->>VNL: "~vnl_vector: flag=false, NO deallocation"
    Note over VNL: "datain leaked"

    Note over Array,VNL: SetSize on non-owning Array
    Caller->>Array: "Array(ext, sz, false) then SetSize(newSz)"
    Array->>VNL: "protected_set_data(nullptr, 0, false)"
    Note right of VNL: "m_LetArrayManageMemory = false"
    Array->>VNL: "set_size(newSz)"
    VNL->>VNL: "vnl_vector_alloc_blah: assert(m_LetArrayManageMemory) FAILS"

Loading

Comments Outside Diff (1)

1. Modules/ThirdParty/VNL/CMakeLists.txt, line 194-195 (link)

   These two mark_as_advanced comments still say "must track ITK LegacyRemove setting", but both variables are now set unconditionally (not gated on ITK_LEGACY_REMOVE / ITK_FUTURE_LEGACY_REMOVE). The comments are misleading for future readers.

_{Reviews (1): Last reviewed commit: "COMP: Adopt VXL private vnl_vector ivars..." | Re-trigger Greptile}

[greptile-apps]

Memory leak when LetArrayManageMemory=true. protected_set_data is called with a hardcoded false, which sets vnl's internal m_LetArrayManageMemory to false. When the caller passes true (meaning the Array should own and eventually free datain), neither the Array destructor nor vnl's destructor will actually deallocate the buffer: ~Array() skips the null-out because m_LetArrayManageMemory=true, and vnl_vector::~vnl_vector() skips deallocation because its own flag is false. The third argument should mirror LetArrayManageMemory.

Suggested change

	vnl_vector<TValue>::protected_set_data(datain, sz, false);
	}
	#if defined(ITK_LEGACY_REMOVE)
	vnl_vector<TValue>::protected_set_data(datain, sz, LetArrayManageMemory);
	}
	#if defined(ITK_LEGACY_REMOVE)

[greptile-apps]

Same ownership mismatch as the constructor and SetData: when LetArrayManageMemory=true, SetDataSameSize should tell vnl to own the new buffer so that the destructor chain actually frees it.

Suggested change

	vnl_vector<TValue>::protected_set_data(datain, this->size(), false);
	m_LetArrayManageMemory = LetArrayManageMemory;
	vnl_vector<TValue>::protected_set_data(datain, this->size(), LetArrayManageMemory);
	m_LetArrayManageMemory = LetArrayManageMemory;

[greptile-apps]

Same ownership-flag mismatch as in the constructor: the third argument is hardcoded to false, so vnl's destructor will never free datain even when LetArrayManageMemory=true. The fix is protected_set_data(datain, sz, LetArrayManageMemory) so that vnl's ownership flag mirrors the caller's intent, exactly as done in VXL's own vnl_vector::set_data helper.

[greptile-apps]

Assertion failure (debug) or silent memory leak (release) when resizing an externally-managed Array. After protected_set_data(nullptr, 0, false), vnl's m_LetArrayManageMemory is false. The subsequent set_size(sz) call takes the else branch (data is null) and invokes vnl_vector_alloc_blah, which immediately asserts this->m_LetArrayManageMemory — that assertion fires because the flag is false. In release builds the assert is a no-op, but m_LetArrayManageMemory stays false, so the newly allocated block is never freed. Passing true correctly signals that ownership of the forthcoming allocation will be held by vnl (and thus by Array).

Suggested change

	if (!m_LetArrayManageMemory)
	{
	vnl_vector<TValue>::data = nullptr;
	vnl_vector<TValue>::protected_set_data(nullptr, 0, false);
	}
	// Call the superclass's set_size
	if (!m_LetArrayManageMemory)
	{
	vnl_vector<TValue>::protected_set_data(nullptr, 0, true);
	}
	// Call the superclass's set_size